| Message ID | 20190110210358.24035-1-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] s390x/pci: Send correct event on hotplug. | expand |
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 <cohuck@redhat.com> 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) 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 :) > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > 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)) { >
On Fri, 11 Jan 2019 08:16:41 +0100 David Hildenbrand <david@redhat.com> 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 <cohuck@redhat.com> Cool, works for me as well. Tested-by: Cornelia Huck <cohuck@redhat.com> 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. > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > --- > > 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)) { > > > >
On 11/01/2019 10:38, Cornelia Huck wrote: > On Fri, 11 Jan 2019 08:16:41 +0100 > David Hildenbrand <david@redhat.com> 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 <cohuck@redhat.com> > > Cool, works for me as well. > > Tested-by: Cornelia Huck <cohuck@redhat.com> > > 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<pmorel@linux.ibm.com> >> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> 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)) { >>> >> >> > >
[restored cc:s] On Mon, 14 Jan 2019 11:06:19 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 11/01/2019 10:38, Cornelia Huck wrote: > > On Fri, 11 Jan 2019 08:16:41 +0100 > > David Hildenbrand <david@redhat.com> 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 <cohuck@redhat.com> > > > > Cool, works for me as well. > > > > Tested-by: Cornelia Huck <cohuck@redhat.com> > > > > 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<pmorel@linux.ibm.com> Thanks for checking. I'd like to queue this, but I'd like an ack from Collin as well. > > > >> > >>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>> --- > >>> 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)) { > >>> > >> > >> > > > > > >
On 1/14/19 12:44 PM, Cornelia Huck wrote: > [restored cc:s] > > On Mon, 14 Jan 2019 11:06:19 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 11/01/2019 10:38, Cornelia Huck wrote: >>> On Fri, 11 Jan 2019 08:16:41 +0100 >>> David Hildenbrand <david@redhat.com> 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 <cohuck@redhat.com> >>> >>> Cool, works for me as well. >>> >>> Tested-by: Cornelia Huck <cohuck@redhat.com> >>> >>> 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<pmorel@linux.ibm.com> > > 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 <walling@linux.ibm.com> >> >> >>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> 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)) { >>>>> >>>> >>>> >>> >>> >> >> > >
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 <pmorel@linux.ibm.com> wrote: >> >>> On 11/01/2019 10:38, Cornelia Huck wrote: >>>> On Fri, 11 Jan 2019 08:16:41 +0100 >>>> David Hildenbrand <david@redhat.com> 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 <cohuck@redhat.com> >>>> >>>> Cool, works for me as well. >>>> >>>> Tested-by: Cornelia Huck <cohuck@redhat.com> >>>> >>>> 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<pmorel@linux.ibm.com> >> >> 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 <walling@linux.ibm.com> Thanks!
On Mon, 14 Jan 2019 21:59:58 +0100 David Hildenbrand <david@redhat.com> 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 <pmorel@linux.ibm.com> wrote: > >> > >>> On 11/01/2019 10:38, Cornelia Huck wrote: > >>>> On Fri, 11 Jan 2019 08:16:41 +0100 > >>>> David Hildenbrand <david@redhat.com> 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 <cohuck@redhat.com> > >>>> > >>>> Cool, works for me as well. > >>>> > >>>> Tested-by: Cornelia Huck <cohuck@redhat.com> > >>>> > >>>> 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<pmorel@linux.ibm.com> > >> > >> 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 <walling@linux.ibm.com> > > Thanks! > Thanks!
On Thu, 10 Jan 2019 22:03:58 +0100 David Hildenbrand <david@redhat.com> 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 <cohuck@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-pci-bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied.
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)) {
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 <cohuck@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/s390x/s390-pci-bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)