Re: [PATCH] pciehp: fast unplug for virtual machines

[Michael S. Tsirkin]

On Sun, Nov 14, 2021 at 05:39:58PM +0100, Lukas Wunner wrote:
> On Thu, Nov 11, 2021 at 10:02:24AM +0100, Gerd Hoffmann wrote:
> > The PCIe specification asks the OS to wait five seconds after the
> 
> The spec reference Bjorn asked for is: PCIe r5.0, sec. 6.7.1.5
> 
> > attention button has been pressed before actually un-plugging the
> > device.  This gives the operator the chance to cancel the operation
> > by pressing the attention button again within those five seconds.
> > 
> > For physical hardware this makes sense.  Picking the wrong button
> > by accident can easily happen and it can be corrected that way.
> > 
> > For virtual hardware the benefits are questionable.  Typically
> > users find the five second delay annoying.
> 
> Why does virtual hardware implement the Attention Button if it's
> perceived as annoying?  Just amend qemu so that it doesn't advertise
> presence of an Attention Button to get rid of the delay.  (Clear the
> Attention Button Present bit in the Slot Capabilities register.)

Because we want ability to request device removal from outside the
guest.

> An Attention Button doesn't make any sense for virtual hardware
> except to test or debug support for it in the kernel.  Just make
> presence of the Attention Button optional and be done with it.
> 
> You'll still be able to bring down the slot in software via the
> "remove" attribute in sysfs.

This requires guest specific code though. Emulating the attention button
works in a guest independent way.

> Same for the 1 second delay in remove_board().  That's mandated by
> PCIe r5.0, sec. 6.7.1.8, but it's only observed if a Power Controller
> is present.  So just clear the Power Controller Present bit in the
> Slot Capabilities register and the delay is gone.
> 
> 
> > @@ -109,6 +110,8 @@ struct controller {
> >  	unsigned int ist_running;
> >  	int request_result;
> >  	wait_queue_head_t requester;
> > +
> > +	bool is_virtual;
> >  };
> 
> This is a quirk for a specific device, so please move it further up to the
> /* capabilities and quirks */ section of struct controller.
> 
> 
> > @@ -227,6 +227,11 @@ static int pciehp_probe(struct pcie_device *dev)
> >  		goto err_out_shutdown_notification;
> >  	}
> >  
> > +	if (dev->port->vendor == PCI_VENDOR_ID_REDHAT &&
> > +	    dev->port->device == 0x000c)
> > +		/* qemu pcie root port */
> > +		ctrl->is_virtual = true;
> > +
> 
> Move this to pcie_init() in pciehp_hpc.c below the existing quirks for
> hotplug_user_indicators and is_thunderbolt.
> 
> 
> > +static bool fast_virtual_unplug = true;
> > +module_param(fast_virtual_unplug, bool, 0644);
> 
> An integer parameter to configure a custom delay would be nicer IMO.
> Of course, anything else than 5 sec deviates from the spec.
> 
> Thanks,
> 
> Lukas