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

[Collin Walling]

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)

Otherwise,

Reviewed-by: Collin Walling <address@hidden>

>>
>>
>>>>  
>>>>> Signed-off-by: David Hildenbrand <address@hidden>
>>>>> ---
>>>>>   hw/s390x/s390-pci-bus.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>>> index 15759b6514..7f911b216a 100644
>>>>> --- a/hw/s390x/s390-pci-bus.c
>>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>>> @@ -899,7 +899,7 @@ static void s390_pcihost_plug(HotplugHandler 
>>>>> *hotplug_dev, DeviceState *dev,
>>>>>           }
>>>>>   
>>>>>           if (dev->hotplugged) {
>>>>> -            s390_pci_generate_plug_event(HP_EVENT_RESERVED_TO_STANDBY,
>>>>> +            s390_pci_generate_plug_event(HP_EVENT_TO_CONFIGURED ,
>>>>>                                            pbdev->fh, pbdev->fid);
>>>>>           }
>>>>>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>>>>>      
>>>>
>>>>  
>>>
>>>   
>>
>>
> 
> 


-- 
Respectfully,
- Collin Walling