Re: [Qemu-ppc] [PATCH] spapr_pci: fix MSI/MSIX selection

[Laurent Vivier]

On 27/01/2018 10:15, David Gibson wrote:
> On Fri, Jan 26, 2018 at 11:25:24PM +0100, Greg Kurz wrote:
>> In various place we don't correctly check if the device supports MSI or
>> MSI-X. This can cause devices to be advertised with MSI support, even
>> if they only support MSI-X (like virtio-pci-* devices for example):
>>
>>                 address@hidden {
>>                         ibm,req#msi = <0x1>; <--- wrong!
>>                      .
>>                      ibm,loc-code = "qemu_virtio-net-pci:0000:00:00.0";
>>                      .
>>                      ibm,req#msi-x = <0x3>;
>>                 };
>>
>> Worse, this can also cause the "ibm,change-msi" RTAS call to corrupt the
>> PCI status and cause migration to fail:
>>
>>   qemu-system-ppc64: get_pci_config_device: Bad config data: i=0x6
>>     read: 0 device: 10 cmask: 10 wmask: 0 w1cmask:0
>>                               ^^
>>            PCI_STATUS_CAP_LIST bit which is assumed to be constant
>>
>> This patch changes spapr_populate_pci_child_dt() to properly check for
>> MSI support using msi_present(): this ensures that PCIDevice::msi_cap
>> was set by msi_init() and that msi_nr_vectors_allocated() will look at
>> the right place in the config space.
>>
>> Checking PCIDevice::msix_entries_nr is enough for MSI-X but let's add
>> a call to msix_present() there as well for consistency.
>>
>> It also changes rtas_ibm_change_msi() to select the appropriate MSI
>> type in Function 1 instead of always selecting plain MSI. This new
>> behaviour is compliant with LoPAPR 1.1, as described in "Table 71.
>> ibm,change-msi Argument Call Buffer":
>>
>>   Function 1: If Number Outputs is equal to 3, request to set to a new
>>            number of MSIs (including set to 0).
>>            If the “ibm,change-msix-capable” property exists and Number
>>            Outputs is equal to 4, request is to set to a new number of
>>            MSI or MSI-X (platform choice) interrupts (including set to
>>            0).
>>
>> Since MSI is the the platform default (LoPAPR 6.2.3 MSI Option), let's
>> check for MSI support first.
>>
>> And finally, it checks the input parameters are valid, as described in
>> LoPAPR 1.1 "R1–7.3.10.5.1–3":
>>
>>   For the MSI option: The platform must return a Status of -3 (Parameter
>>   error) from ibm,change-msi, with no change in interrupt assignments if
>>   the PCI configuration address does not support MSI and Function 3 was
>>   requested (that is, the “ibm,req#msi” property must exist for the PCI
>>   configuration address in order to use Function 3), or does not support
>>   MSI-X and Function 4 is requested (that is, the “ibm,req#msi-x” property
>>   must exist for the PCI configuration address in order to use Function 4),
>>   or if neither MSIs nor MSI-Xs are supported and Function 1 is requested.
>>
>> This ensures that the ret_intr_type variable contains a valid MSI type
>> for this device, and that spapr_msi_setmsg() won't corrupt the PCI status.
>>
>> Signed-off-by: Greg Kurz <address@hidden>
> 
> Applied, thanks.
> 
> Alexey, is this the migration bug you were mentioning to me?
> 
> +lvivier
> 
> Laurent, could this cover any of the migration bugs you're looking at?
> If not we should probably file a new downstream BZ for it.

It doesn't fix my problem:. I have always this kind of error after a
migration on P9:

[   39.305470] Unable to handle kernel paging request for d6
[   39.305534] Faulting instruction address: 0xc000000000694ac0

[   39.305578] Oops: Kernel access of bad area, sig: 11 [#1]
...
[   39.306625] NIP [c000000000694ac0] ioread16+0x30/0x1a0

[   39.306655] LR [c008000000bb074c] vp_get+0x15c/0x190 [virtio_pci]

[   39.306690] Call Trace:

[   39.306707] [c00000000315fb50] [c00000000001c9c0]
__switch_to+0x330/0x660 (u)
[   39.306761] [c00000000315fbc0] [c008000000bb074c] vp_get+0x15c/0x190
[virtio]
[   39.306812] [c00000000315fc00] [c008000000d41328]
virtnet_config_changed_wor]

Greg, do you have a test case for the bug your patch fixes?

Thanks,
Laurent