Re: [Qemu-devel] [qemu-s390x] [PATCH v1] s390x/pci: Send correct event on hotplug.

[David Hildenbrand]

On 14.01.19 21:00, Collin Walling wrote:
> On 1/14/19 12:44 PM, Cornelia Huck wrote:
>> [restored cc:s]
>>
>> On Mon, 14 Jan 2019 11:06:19 +0100
>> Pierre Morel <address@hidden> wrote:
>>
>>> On 11/01/2019 10:38, Cornelia Huck wrote:
>>>> On Fri, 11 Jan 2019 08:16:41 +0100
>>>> David Hildenbrand <address@hidden> wrote:
>>>>   
>>>>> On 10.01.19 22:03, David Hildenbrand wrote:  
>>>>>> Comit 2c28c490571f ("s390x/pci: let pci devices start in configured 
>>>>>> mode")
>>>>>> changed the initial state of zPCI devices from ZPCI_FS_STANDBY to
>>>>>> ZPCI_FS_DISABLED (a.k.a. configured). However we still only send a
>>>>>> HP_EVENT_RESERVED_TO_STANDBY event to the guest, indicating a wrong
>>>>>> state.
>>>>>>
>>>>>> Let's send a HP_EVENT_TO_CONFIGURED event instead, to match the actual
>>>>>> state the device is in.
>>>>>>
>>>>>> This fixes hotplugged devices having to be enabled explicitly in the
>>>>>> guest e.g. via echo 1 > /sys/bus/pci/slots/00000000/power.
>>>>>>
>>>>>> Fixes: 2c28c490571f ("s390x/pci: let pci devices start in configured 
>>>>>> mode")
>>>>>> Report-by: Cornelia Huck <address@hidden>  
>>>>
>>>> Cool, works for me as well.
>>>>
>>>> Tested-by: Cornelia Huck <address@hidden>
>>>>
>>>> Do we want to cc:stable? Probably not, as it's more annoying than
>>>> critical, and pci hotplug does not seem to be much used on s390x.
>>>>   
>>>>>
>>>>> If this patch is the right thing to do, then
>>>>>
>>>>> 1. s/Report-by/Reported-by/
>>>>> 2. Dropping the "." from the subject
>>>>>
>>>>> (yes, it was late)  
>>>>
>>>> :) Can do while applying.
>>>>   
>>>>>
>>>>> I wonder if we should do both events sequentially, but as I don't have
>>>>> access to the architecture I have to rely on that this works :)  
>>>>
>>>> Yep, let's wait for feedback from folks with architecture access.
>>>>   
>>>
>>> Works fine on the architecture too.
>>>
>>> Seems the logical thing to do for me.
>>>
>>> Reviewed-by: Pierre Morel<address@hidden>
>>
>> Thanks for checking.
>>
>> I'd like to queue this, but I'd like an ack from Collin as well.
>>
> 
> Would you mind adding a comment somewhere that states something like
> "we can safely bypass the standby state when PCI hotplugging for a guest" 
> just to be clear that QEMU is a bit different from how we handle it on the 
> LPAR
> level?
> 
> That comment would more-or-less clarify why we set the ZPCI_FS_<STATE> 
> directly
> to disabled instead of to standby when hotplugging (which, AFAIU, is the 
> order 
> how things occur at the LPAR level)

This patch relies on Christians patch, where the general concept was
explained. As we changed the initial state, we have to send a
corresponding hotplug event. But still we can add a comment to shine
some light on the general concept.

@Conny, can you add after the first paragraph: (or let me know if you
want a respin)

"On real HW, a PCI device always pops up in the STANDBY state. In QEMU,
we decided to let it show up directly in the configured state (as
configuring it is otherwise just an extra burden for the admin). We can
safely bypass the STANDBY state when hotplugging PCI devices to a guest."

> 
> Otherwise,
> 
> Reviewed-by: Collin Walling <address@hidden>

Thanks!

-- 

Thanks,

David / dhildenb