On Thu, Jan 10, 2019 at 06:09:23PM -0800, si-wei liu wrote:
>
>
> On 01/09/2019 07:56 AM, Michael S. Tsirkin wrote:
> > On Mon, Jan 07, 2019 at 05:29:41PM -0500, Venu Busireddy wrote:
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index 80d42e1..2a3ffd3 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -176,6 +176,25 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s,
> > > unsigned bsel, unsigned slo
> > > }
> > > }
> > > +static void acpi_pcihp_cleanup_failover_primary(AcpiPciHpState *s, int
> > > bsel)
> > > +{
> > > + BusChild *kid, *next;
> > > + PCIBus *bus = acpi_pcihp_find_hotplug_bus(s, bsel);
> > > +
> > > + if (!bus) {
> > > + return;
> > > + }
> > > + QTAILQ_FOREACH_SAFE(kid, &bus->qbus.children, sibling, next) {
> > > + DeviceState *qdev = kid->child;
> > > + PCIDevice *pdev = PCI_DEVICE(qdev);
> > > + int slot = PCI_SLOT(pdev->devfn);
> > > +
> > > + if (pdev->failover_primary) {
> > > + s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
> > > + }
> > > + }
> > > +}
> > > +
> > > static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)
> > > {
> > > BusChild *kid, *next;
> > So the result here will be that device will be deleted completely,
> > and will not reappear after guest reboot.
> The management stack will replug the VF until seeing the STANDBY_CHANGED
> "enabled" event after guest driver finishes feature negotiation and sets
> driver_ok.
>
> > I don't think this is what we wanted.
> > I think we wanted a special state that will hide device from guest until
> > guest acks the failover bit.
> What do we get by hiding? On the next reboot after system reset guest may
> load an older OS instance without standby advertised. The VF can't be
> plugged out then?
>
> The model we adopt here doesn't pair virtio with VF in the QEMU level. If
> the VF isn't being used by guest, it would make sense to notify management
> to release VF anyways.
Hmm it's different from what I envisioned and more work for management,
but maybe it's ok ... I will need to think about it.
> >
> >
> > > @@ -207,6 +226,14 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> > > int i;
> > > for (i = 0; i < ACPI_PCIHP_MAX_HOTPLUG_BUS; ++i) {
> > > + /*
> > > + * Set the acpi_pcihp_pci_status[].down bits of all the
> > > + * failover_primary devices so that the devices are ejected
> > > + * from the guest. We can't use the qdev_unplug() as well as the
> > > + * hotplug_handler to unplug the devices, because the guest may
> > > + * not be in a state to cooperate.
> > > + */
> > > + acpi_pcihp_cleanup_failover_primary(s, i);
> > > acpi_pcihp_update_hotplug_bus(s, i);
> > > }
> > > }
> > I really don't want acpi to know anything about failover.
> >
> > All that needs to happen is sending a device delete request
> > to guest. Should work with any hotplug removal:
> > pci standard,acpi, etc.
> >
> As the code comments above indicated, there was issue uncovered that the
> guest may not be in a state to respond to interrupt during reboot.
If you request removal then hotplug machinery normally will eject
the device on system reset. You need to request it early enough though.
I guess this missing is what happened.
> Actually
> management stack running fast enough is supposed to do this graceful hot
> plug removal upon receiving the STANDBY_CHANGED "disabled" event. However,
> if management stack's unable to do so, the code here makes sure the VF can
> be deleted and won't be seen by an older kernel after reboot.
>
> -Siwei
I'm sorry I don't understand. On a system with PCIe native hotplug
poking at ACPI is just wrong.
--
MST
