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

[Cornelia Huck]

On Mon, 14 Jan 2019 21:59:58 +0100
David Hildenbrand <address@hidden> wrote:

> 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."

I'll just add that text.

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

Thanks!