Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540331 > unrolled thread

Re: [PATCH] vfio/pci: Support error recovery

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] vfio/pci: Support error recovery Cao jin <caoj.fnst@cn.fujitsu.com> - 2016-12-12 14:50 +0100
    Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-12 20:20 +0100
      Re: [PATCH] vfio/pci: Support error recovery "Michael S. Tsirkin" <mst@redhat.com> - 2016-12-12 23:30 +0100
        Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-12 23:50 +0100
          Re: [PATCH] vfio/pci: Support error recovery "Michael S. Tsirkin" <mst@redhat.com> - 2016-12-13 04:20 +0100
            Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-13 04:50 +0100
              Re: [PATCH] vfio/pci: Support error recovery "Michael S. Tsirkin" <mst@redhat.com> - 2016-12-13 17:20 +0100
                Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-13 18:20 +0100
                  Re: [PATCH] vfio/pci: Support error recovery "Michael S. Tsirkin" <mst@redhat.com> - 2016-12-14 03:00 +0100
                    Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-14 04:10 +0100
                      Re: [PATCH] vfio/pci: Support error recovery "Michael S. Tsirkin" <mst@redhat.com> - 2016-12-14 23:30 +0100
                        Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-14 23:50 +0100
                          Re: [PATCH] vfio/pci: Support error recovery "Michael S. Tsirkin" <mst@redhat.com> - 2016-12-15 00:10 +0100
                            Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-15 00:40 +0100
      Re: [PATCH] vfio/pci: Support error recovery Cao jin <caoj.fnst@cn.fujitsu.com> - 2016-12-14 11:30 +0100
        Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-14 23:20 +0100
          Re: [PATCH] vfio/pci: Support error recovery "Michael S. Tsirkin" <mst@redhat.com> - 2016-12-14 23:30 +0100
            Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-15 00:00 +0100
          Re: [PATCH] vfio/pci: Support error recovery Cao jin <caoj.fnst@cn.fujitsu.com> - 2016-12-15 15:00 +0100
            Re: [PATCH] vfio/pci: Support error recovery "Michael S. Tsirkin" <mst@redhat.com> - 2016-12-15 16:00 +0100
              Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-15 23:10 +0100
                Re: [PATCH] vfio/pci: Support error recovery Cao jin <caoj.fnst@cn.fujitsu.com> - 2016-12-16 11:20 +0100
              Re: [PATCH] vfio/pci: Support error recovery Cao jin <caoj.fnst@cn.fujitsu.com> - 2016-12-16 11:20 +0100
            Re: [PATCH] vfio/pci: Support error recovery Alex Williamson <alex.williamson@redhat.com> - 2016-12-15 18:10 +0100

Page 1 of 2  [1] 2  Next page →

#1540331 — Re: [PATCH] vfio/pci: Support error recovery

Hi,
I have 2 solutions(high level design) came to me, please see if they are
acceptable, or which one is acceptable. Also have some questions.

1. block guest access during host recovery

   add new field error_recovering in struct vfio_pci_device to
   indicate host recovery status. aer driver in host will still do
   reset link

   - set error_recovering in vfio-pci driver's error_detected, used to
     block all kinds of user access(config space, mmio)
   - in order to solve concurrent issue of device resetting & user
     access, check device state[*] in vfio-pci driver's resume, see if
     device reset is done, if it is, then clear"error_recovering", or
     else new a timer, check device state periodically until device
     reset is done. (what if device reset don't end for a long time?)
   - In qemu, translate guest link reset to host link reset.
     A question here: we already have link reset in host, is a second
     link reset necessary? why?
 
   [*] how to check device state: reading certain config space
       register, check return value is valid or not(All F's)

2. skip link reset in aer driver of host kernel, for vfio-pci.
   Let user decide how to do serious recovery

   add new field "user_driver" in struct pci_dev, used to skip link
   reset for vfio-pci; add new field "link_reset" in struct
   vfio_pci_device to indicate link has been reset or not during
   recovery

   - set user_driver in vfio_pci_probe(), to skip link reset for
     vfio-pci in host.
   - (use a flag)block user access(config, mmio) during host recovery
     (not sure if this step is necessary)
   - In qemu, translate guest link reset to host link reset.
   - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
     is executed
   - In vfio-pci driver's resume, new a timer, check "link_reset" field
     periodically, if it is set in reasonable time, then clear it and
     delete timer, or else, vfio-pci driver will does the link reset!


A quick question:
I don't know how devices is divided into iommu groups, is it possible
for functions in a multi-function device to be split into different groups?

-- 
Sincerely,
Cao jin


On 11/27/2016 07:34 PM, Cao jin wrote:
> It is user space driver's or device-specific driver's(in guest) responsbility
> to do a serious recovery when error happened. Link-reset is one part of
> recovery, when pci device is assigned to VM via vfio, link-reset will do
> twice in host & guest separately, which will cause many trouble for a
> successful recovery, so, disable the vfio-pci's link-reset in aer driver
> in host, this is a keypoint for guest to do error recovery successfully.
> 
> CC: alex.williamson@redhat.com
> CC: mst@redhat.com
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> This is actually a RFC version(has debug lines left), and has minor changes in
> aer driver, so I think maybe it is better not to CC pci guys in this round.
> Later will do.
> 
>  drivers/pci/pcie/aer/aerdrv_core.c  | 12 ++++++-
>  drivers/vfio/pci/vfio_pci.c         | 63 +++++++++++++++++++++++++++++++++++--
>  drivers/vfio/pci/vfio_pci_private.h |  2 ++
>  3 files changed, 74 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 521e39c..289fb8e 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -496,7 +496,17 @@ static void do_recovery(struct pci_dev *dev, int severity)
>  			"error_detected",
>  			report_error_detected);
>  
> -	if (severity == AER_FATAL) {
> +	/* vfio-pci as a general meta driver, it actually couldn't do any real
> +	 * recovery for device. It is user space driver, or device-specific
> +	 * driver in guest who should take care of the serious error recovery,
> +	 * link reset actually is one part of whole recovery. Doing reset_link
> +	 * in aer driver of host kernel for vfio-pci devices will cause many
> +	 * trouble for user space driver or guest's device-specific driver,
> +	 * for example: the serious recovery often need to read register in
> +	 * config space, but if register reading happens during link-resetting,
> +	 * it is quite possible to return invalid value like all F's, which
> +	 * will result in unpredictable error. */
> +	if (severity == AER_FATAL && strcmp(dev->driver->name, "vfio-pci")) {
>  		result = reset_link(dev);
>  		if (result != PCI_ERS_RESULT_RECOVERED)
>  			goto failed;
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 712a849..aefd751 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -535,6 +535,15 @@ static long vfio_pci_ioctl(void *device_data,
>  	struct vfio_pci_device *vdev = device_data;
>  	unsigned long minsz;
>  
> +	if (vdev->aer_recovering && (cmd == VFIO_DEVICE_SET_IRQS ||
> +	    cmd == VFIO_DEVICE_RESET || cmd == VFIO_DEVICE_PCI_HOT_RESET)) {
> +		int ret;
> +		ret = wait_for_completion_interruptible(
> +			&vdev->aer_error_completion);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (cmd == VFIO_DEVICE_GET_INFO) {
>  		struct vfio_device_info info;
>  
> @@ -1117,6 +1126,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	vdev->irq_type = VFIO_PCI_NUM_IRQS;
>  	mutex_init(&vdev->igate);
>  	spin_lock_init(&vdev->irqlock);
> +	init_completion(&vdev->aer_error_completion);
>  
>  	ret = vfio_add_group_dev(&pdev->dev, &vfio_pci_ops, vdev);
>  	if (ret) {
> @@ -1176,6 +1186,9 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
>  {
>  	struct vfio_pci_device *vdev;
>  	struct vfio_device *device;
> +	u32 uncor_status = 0;
> +	unsigned int aer_cap_offset = 0;
> +	int ret;
>  
>  	device = vfio_device_get_from_dev(&pdev->dev);
>  	if (device == NULL)
> @@ -1187,10 +1200,30 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
>  		return PCI_ERS_RESULT_DISCONNECT;
>  	}
>  
> +	/* get device's uncorrectable error status as soon as possible,
> +	 * and signal it to user space. The later we read it, the possibility
> +	 * the register value is mangled grows. */
> +	aer_cap_offset = pci_find_ext_capability(vdev->pdev, PCI_EXT_CAP_ID_ERR);
> +	ret = pci_read_config_dword(vdev->pdev, aer_cap_offset + 
> +                                    PCI_ERR_UNCOR_STATUS, &uncor_status);
> +        if (ret)
> +                return PCI_ERS_RESULT_DISCONNECT;
> +
> +	pr_err("device %d got AER detect notification. uncorrectable error status = 0x%x\n", pdev->devfn, uncor_status);//to be removed
>  	mutex_lock(&vdev->igate);
> +    
> +	vdev->aer_recovering = true;
> +	reinit_completion(&vdev->aer_error_completion);
> +
> +	/* suspend config space access from user space,
> +	 * when vfio-pci's error recovery process is on */
> +	pci_cfg_access_trylock(vdev->pdev);
>  
> -	if (vdev->err_trigger)
> -		eventfd_signal(vdev->err_trigger, 1);
> +	if (vdev->err_trigger && uncor_status) {
> +		pr_err("device %d signal uncor status to user space", pdev->devfn);//may be removed
> +		/* signal uncorrectable error status to user space */
> +		eventfd_signal(vdev->err_trigger, uncor_status);
> +        }
>  
>  	mutex_unlock(&vdev->igate);
>  
> @@ -1199,8 +1232,34 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
>  	return PCI_ERS_RESULT_CAN_RECOVER;
>  }
>  
> +static void vfio_pci_aer_resume(struct pci_dev *pdev)
> +{
> +	struct vfio_pci_device *vdev;
> +	struct vfio_device *device;
> +
> +	device = vfio_device_get_from_dev(&pdev->dev);
> +	if (device == NULL)
> +		return;
> +
> +	vdev = vfio_device_data(device);
> +	if (vdev == NULL) {
> +		vfio_device_put(device);
> +		return;
> +	}
> +
> +	/* vfio-pci's error recovery is done, time to
> +	 * resume pci config space's accesses */
> +	pci_cfg_access_unlock(vdev->pdev);
> +
> +	vdev->aer_recovering = false;
> +	complete_all(&vdev->aer_error_completion);
> +
> +	vfio_device_put(device);
> +}
> +
>  static const struct pci_error_handlers vfio_err_handlers = {
>  	.error_detected = vfio_pci_aer_err_detected,
> +	.resume         = vfio_pci_aer_resume,
>  };
>  
>  static struct pci_driver vfio_pci_driver = {
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index 8a7d546..ebf1041 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -83,6 +83,8 @@ struct vfio_pci_device {
>  	bool			bardirty;
>  	bool			has_vga;
>  	bool			needs_reset;
> +	bool			aer_recovering;
> +	struct completion	aer_error_completion;
>  	struct pci_saved_state	*pci_saved_state;
>  	int			refcnt;
>  	struct eventfd_ctx	*err_trigger;
> 

[toc] | [next] | [standalone]

#1540564

On Mon, 12 Dec 2016 21:49:01 +0800
Cao jin <caoj.fnst@cn.fujitsu.com> wrote:

> Hi,
> I have 2 solutions(high level design) came to me, please see if they are
> acceptable, or which one is acceptable. Also have some questions.
> 
> 1. block guest access during host recovery
> 
>    add new field error_recovering in struct vfio_pci_device to
>    indicate host recovery status. aer driver in host will still do
>    reset link
> 
>    - set error_recovering in vfio-pci driver's error_detected, used to
>      block all kinds of user access(config space, mmio)
>    - in order to solve concurrent issue of device resetting & user
>      access, check device state[*] in vfio-pci driver's resume, see if
>      device reset is done, if it is, then clear"error_recovering", or
>      else new a timer, check device state periodically until device
>      reset is done. (what if device reset don't end for a long time?)
>    - In qemu, translate guest link reset to host link reset.
>      A question here: we already have link reset in host, is a second
>      link reset necessary? why?
>  
>    [*] how to check device state: reading certain config space
>        register, check return value is valid or not(All F's)

Isn't this exactly the path we were on previously?  There might be an
optimization that we could skip back-to-back resets, but how can you
necessarily infer that the resets are for the same thing?  If the user
accesses the device between resets, can you still guarantee the guest
directed reset is unnecessary?  If time passes between resets, do you
know they're for the same event?  How much time can pass between the
host and guest reset to know they're for the same event?  In the
process of error handling, which is more important, speed or
correctness?
 
> 2. skip link reset in aer driver of host kernel, for vfio-pci.
>    Let user decide how to do serious recovery
> 
>    add new field "user_driver" in struct pci_dev, used to skip link
>    reset for vfio-pci; add new field "link_reset" in struct
>    vfio_pci_device to indicate link has been reset or not during
>    recovery
> 
>    - set user_driver in vfio_pci_probe(), to skip link reset for
>      vfio-pci in host.
>    - (use a flag)block user access(config, mmio) during host recovery
>      (not sure if this step is necessary)
>    - In qemu, translate guest link reset to host link reset.
>    - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
>      is executed
>    - In vfio-pci driver's resume, new a timer, check "link_reset" field
>      periodically, if it is set in reasonable time, then clear it and
>      delete timer, or else, vfio-pci driver will does the link reset!

What happens in the case of a multifunction device where each function
is part of a separate IOMMU group and one function is hot-removed from
the user?  We can't do a link reset on that function since the other
function is still in use.  We have no choice but release a device in an
unknown state back to the host.  As previously discussed, we don't
expect that any sort of function-level FLR will necessarily reset the
device to the same state.  I also don't really like vfio-pci taking
over error handling capabilities from the PCI-core.  That's redundant
code and extra maintenance overhead.
 
> A quick question:
> I don't know how devices is divided into iommu groups, is it possible
> for functions in a multi-function device to be split into different groups?

Yes, if a multifunction device supports ACS or if we have quirks to
expose that the functions do not perform internal peer-to-peer, then
they may be in separate IOMMU groups, depending on the rest of the PCI
topology.  See:

http://vfio.blogspot.com/2014/08/iommu-groups-inside-and-out.html

Thanks,
Alex

[toc] | [prev] | [next] | [standalone]

#1540703

On Mon, Dec 12, 2016 at 12:12:16PM -0700, Alex Williamson wrote:
> On Mon, 12 Dec 2016 21:49:01 +0800
> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> 
> > Hi,
> > I have 2 solutions(high level design) came to me, please see if they are
> > acceptable, or which one is acceptable. Also have some questions.
> > 
> > 1. block guest access during host recovery
> > 
> >    add new field error_recovering in struct vfio_pci_device to
> >    indicate host recovery status. aer driver in host will still do
> >    reset link
> > 
> >    - set error_recovering in vfio-pci driver's error_detected, used to
> >      block all kinds of user access(config space, mmio)
> >    - in order to solve concurrent issue of device resetting & user
> >      access, check device state[*] in vfio-pci driver's resume, see if
> >      device reset is done, if it is, then clear"error_recovering", or
> >      else new a timer, check device state periodically until device
> >      reset is done. (what if device reset don't end for a long time?)
> >    - In qemu, translate guest link reset to host link reset.
> >      A question here: we already have link reset in host, is a second
> >      link reset necessary? why?
> >  
> >    [*] how to check device state: reading certain config space
> >        register, check return value is valid or not(All F's)
> 
> Isn't this exactly the path we were on previously?  There might be an
> optimization that we could skip back-to-back resets, but how can you
> necessarily infer that the resets are for the same thing?  If the user
> accesses the device between resets, can you still guarantee the guest
> directed reset is unnecessary?  If time passes between resets, do you
> know they're for the same event?  How much time can pass between the
> host and guest reset to know they're for the same event?  In the
> process of error handling, which is more important, speed or
> correctness?
>  
> > 2. skip link reset in aer driver of host kernel, for vfio-pci.
> >    Let user decide how to do serious recovery
> > 
> >    add new field "user_driver" in struct pci_dev, used to skip link
> >    reset for vfio-pci; add new field "link_reset" in struct
> >    vfio_pci_device to indicate link has been reset or not during
> >    recovery
> > 
> >    - set user_driver in vfio_pci_probe(), to skip link reset for
> >      vfio-pci in host.
> >    - (use a flag)block user access(config, mmio) during host recovery
> >      (not sure if this step is necessary)
> >    - In qemu, translate guest link reset to host link reset.
> >    - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
> >      is executed
> >    - In vfio-pci driver's resume, new a timer, check "link_reset" field
> >      periodically, if it is set in reasonable time, then clear it and
> >      delete timer, or else, vfio-pci driver will does the link reset!
> 
> What happens in the case of a multifunction device where each function
> is part of a separate IOMMU group and one function is hot-removed from
> the user?

So just don't do it then. Topology must match between host and guest,
except maybe for the case of devices with host driver  (e.g. PF)
which we might be able to synchronize against.

>  We can't do a link reset on that function since the other
> function is still in use.  We have no choice but release a device in an
> unknown state back to the host.  As previously discussed, we don't
> expect that any sort of function-level FLR will necessarily reset the
> device to the same state.  I also don't really like vfio-pci taking
> over error handling capabilities from the PCI-core.  That's redundant
> code and extra maintenance overhead.
>  
> > A quick question:
> > I don't know how devices is divided into iommu groups, is it possible
> > for functions in a multi-function device to be split into different groups?
> 
> Yes, if a multifunction device supports ACS or if we have quirks to
> expose that the functions do not perform internal peer-to-peer, then
> they may be in separate IOMMU groups, depending on the rest of the PCI
> topology.  See:
> 
> http://vfio.blogspot.com/2014/08/iommu-groups-inside-and-out.html
> 
> Thanks,
> Alex

[toc] | [prev] | [next] | [standalone]

#1540720

On Tue, 13 Dec 2016 00:29:42 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 12, 2016 at 12:12:16PM -0700, Alex Williamson wrote:
> > On Mon, 12 Dec 2016 21:49:01 +0800
> > Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> >   
> > > Hi,
> > > I have 2 solutions(high level design) came to me, please see if they are
> > > acceptable, or which one is acceptable. Also have some questions.
> > > 
> > > 1. block guest access during host recovery
> > > 
> > >    add new field error_recovering in struct vfio_pci_device to
> > >    indicate host recovery status. aer driver in host will still do
> > >    reset link
> > > 
> > >    - set error_recovering in vfio-pci driver's error_detected, used to
> > >      block all kinds of user access(config space, mmio)
> > >    - in order to solve concurrent issue of device resetting & user
> > >      access, check device state[*] in vfio-pci driver's resume, see if
> > >      device reset is done, if it is, then clear"error_recovering", or
> > >      else new a timer, check device state periodically until device
> > >      reset is done. (what if device reset don't end for a long time?)
> > >    - In qemu, translate guest link reset to host link reset.
> > >      A question here: we already have link reset in host, is a second
> > >      link reset necessary? why?
> > >  
> > >    [*] how to check device state: reading certain config space
> > >        register, check return value is valid or not(All F's)  
> > 
> > Isn't this exactly the path we were on previously?  There might be an
> > optimization that we could skip back-to-back resets, but how can you
> > necessarily infer that the resets are for the same thing?  If the user
> > accesses the device between resets, can you still guarantee the guest
> > directed reset is unnecessary?  If time passes between resets, do you
> > know they're for the same event?  How much time can pass between the
> > host and guest reset to know they're for the same event?  In the
> > process of error handling, which is more important, speed or
> > correctness?
> >    
> > > 2. skip link reset in aer driver of host kernel, for vfio-pci.
> > >    Let user decide how to do serious recovery
> > > 
> > >    add new field "user_driver" in struct pci_dev, used to skip link
> > >    reset for vfio-pci; add new field "link_reset" in struct
> > >    vfio_pci_device to indicate link has been reset or not during
> > >    recovery
> > > 
> > >    - set user_driver in vfio_pci_probe(), to skip link reset for
> > >      vfio-pci in host.
> > >    - (use a flag)block user access(config, mmio) during host recovery
> > >      (not sure if this step is necessary)
> > >    - In qemu, translate guest link reset to host link reset.
> > >    - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
> > >      is executed
> > >    - In vfio-pci driver's resume, new a timer, check "link_reset" field
> > >      periodically, if it is set in reasonable time, then clear it and
> > >      delete timer, or else, vfio-pci driver will does the link reset!  
> > 
> > What happens in the case of a multifunction device where each function
> > is part of a separate IOMMU group and one function is hot-removed from
> > the user?  
> 
> So just don't do it then. Topology must match between host and guest,
> except maybe for the case of devices with host driver  (e.g. PF)
> which we might be able to synchronize against.

We're talking about host kernel level handling here.  The host kernel
cannot defer the link reset to the user under the assumption that the
user is handling the devices in a very specific way.  The moment we do
that, we've lost.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1540814

On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:
> > So just don't do it then. Topology must match between host and guest,
> > except maybe for the case of devices with host driver  (e.g. PF)
> > which we might be able to synchronize against.
> 
> We're talking about host kernel level handling here.  The host kernel
> cannot defer the link reset to the user under the assumption that the
> user is handling the devices in a very specific way.  The moment we do
> that, we've lost.

The way is same as baremetal though, so why not?

And if user doesn't do what's expected, we can
do the full link reset on close.

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1540824

On Tue, 13 Dec 2016 05:15:13 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:
> > > So just don't do it then. Topology must match between host and guest,
> > > except maybe for the case of devices with host driver  (e.g. PF)
> > > which we might be able to synchronize against.  
> > 
> > We're talking about host kernel level handling here.  The host kernel
> > cannot defer the link reset to the user under the assumption that the
> > user is handling the devices in a very specific way.  The moment we do
> > that, we've lost.  
> 
> The way is same as baremetal though, so why not?

How do we know this?  What if the user is dpdk?  The kernel is
responsible for maintaining the integrity of the system and devices,
not the user.

> And if user doesn't do what's expected, we can
> do the full link reset on close.

That's exactly my point, if we're talking about multiple devices,
there's no guarantee that the close() for each is simultaneous.  If one
function is released before the other we cannot do a bus reset.  If
that device is then opened by another user before its sibling is
released, then we once again cannot perform a link reset.  I don't
think it would be reasonable to mark the released device quarantined
until the sibling is released, that would be a terrible user experience.

[toc] | [prev] | [next] | [standalone]

#1541229

On Mon, Dec 12, 2016 at 08:39:48PM -0700, Alex Williamson wrote:
> On Tue, 13 Dec 2016 05:15:13 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:
> > > > So just don't do it then. Topology must match between host and guest,
> > > > except maybe for the case of devices with host driver  (e.g. PF)
> > > > which we might be able to synchronize against.  
> > > 
> > > We're talking about host kernel level handling here.  The host kernel
> > > cannot defer the link reset to the user under the assumption that the
> > > user is handling the devices in a very specific way.  The moment we do
> > > that, we've lost.  
> > 
> > The way is same as baremetal though, so why not?
> 
> How do we know this?  What if the user is dpdk?  The kernel is
> responsible for maintaining the integrity of the system and devices,
> not the user.
> 
> > And if user doesn't do what's expected, we can
> > do the full link reset on close.
> 
> That's exactly my point, if we're talking about multiple devices,
> there's no guarantee that the close() for each is simultaneous.  If one
> function is released before the other we cannot do a bus reset.  If
> that device is then opened by another user before its sibling is
> released, then we once again cannot perform a link reset.  I don't
> think it would be reasonable to mark the released device quarantined
> until the sibling is released, that would be a terrible user experience.

Not sure why you find it so terrible, and I don't think there's another way.

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1541286

On Tue, 13 Dec 2016 18:12:34 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 12, 2016 at 08:39:48PM -0700, Alex Williamson wrote:
> > On Tue, 13 Dec 2016 05:15:13 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:  
> > > > > So just don't do it then. Topology must match between host and guest,
> > > > > except maybe for the case of devices with host driver  (e.g. PF)
> > > > > which we might be able to synchronize against.    
> > > > 
> > > > We're talking about host kernel level handling here.  The host kernel
> > > > cannot defer the link reset to the user under the assumption that the
> > > > user is handling the devices in a very specific way.  The moment we do
> > > > that, we've lost.    
> > > 
> > > The way is same as baremetal though, so why not?  
> > 
> > How do we know this?  What if the user is dpdk?  The kernel is
> > responsible for maintaining the integrity of the system and devices,
> > not the user.
> >   
> > > And if user doesn't do what's expected, we can
> > > do the full link reset on close.  
> > 
> > That's exactly my point, if we're talking about multiple devices,
> > there's no guarantee that the close() for each is simultaneous.  If one
> > function is released before the other we cannot do a bus reset.  If
> > that device is then opened by another user before its sibling is
> > released, then we once again cannot perform a link reset.  I don't
> > think it would be reasonable to mark the released device quarantined
> > until the sibling is released, that would be a terrible user experience.  
> 
> Not sure why you find it so terrible, and I don't think there's another way.

If we can't do it without regressing the support we currently have,
let's not do it at all.

[toc] | [prev] | [next] | [standalone]

#1541639

On Tue, Dec 13, 2016 at 09:27:59AM -0700, Alex Williamson wrote:
> On Tue, 13 Dec 2016 18:12:34 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Dec 12, 2016 at 08:39:48PM -0700, Alex Williamson wrote:
> > > On Tue, 13 Dec 2016 05:15:13 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:  
> > > > > > So just don't do it then. Topology must match between host and guest,
> > > > > > except maybe for the case of devices with host driver  (e.g. PF)
> > > > > > which we might be able to synchronize against.    
> > > > > 
> > > > > We're talking about host kernel level handling here.  The host kernel
> > > > > cannot defer the link reset to the user under the assumption that the
> > > > > user is handling the devices in a very specific way.  The moment we do
> > > > > that, we've lost.    
> > > > 
> > > > The way is same as baremetal though, so why not?  
> > > 
> > > How do we know this?  What if the user is dpdk?  The kernel is
> > > responsible for maintaining the integrity of the system and devices,
> > > not the user.
> > >   
> > > > And if user doesn't do what's expected, we can
> > > > do the full link reset on close.  
> > > 
> > > That's exactly my point, if we're talking about multiple devices,
> > > there's no guarantee that the close() for each is simultaneous.  If one
> > > function is released before the other we cannot do a bus reset.  If
> > > that device is then opened by another user before its sibling is
> > > released, then we once again cannot perform a link reset.  I don't
> > > think it would be reasonable to mark the released device quarantined
> > > until the sibling is released, that would be a terrible user experience.  
> > 
> > Not sure why you find it so terrible, and I don't think there's another way.
> 
> If we can't do it without regressing the support we currently have,
> let's not do it at all.

Why would we regress?  As long as there are no unrecoverable errors,
there's no need to change behaviour at all.

Alex, do you have a picture of how error recovery can work in your mind?
Your answers seem to imply you do, and these patches don't implement
this correctly.  I'm not sure about others, but I for one am unable to
piece it together from the comments you provide.  If yes, could you
maybe do a short writeup of an architecture you would be comfortable
with?

Thanks,

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1541669

On Wed, 14 Dec 2016 03:58:17 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 13, 2016 at 09:27:59AM -0700, Alex Williamson wrote:
> > On Tue, 13 Dec 2016 18:12:34 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Mon, Dec 12, 2016 at 08:39:48PM -0700, Alex Williamson wrote:  
> > > > On Tue, 13 Dec 2016 05:15:13 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:    
> > > > > > > So just don't do it then. Topology must match between host and guest,
> > > > > > > except maybe for the case of devices with host driver  (e.g. PF)
> > > > > > > which we might be able to synchronize against.      
> > > > > > 
> > > > > > We're talking about host kernel level handling here.  The host kernel
> > > > > > cannot defer the link reset to the user under the assumption that the
> > > > > > user is handling the devices in a very specific way.  The moment we do
> > > > > > that, we've lost.      
> > > > > 
> > > > > The way is same as baremetal though, so why not?    
> > > > 
> > > > How do we know this?  What if the user is dpdk?  The kernel is
> > > > responsible for maintaining the integrity of the system and devices,
> > > > not the user.
> > > >     
> > > > > And if user doesn't do what's expected, we can
> > > > > do the full link reset on close.    
> > > > 
> > > > That's exactly my point, if we're talking about multiple devices,
> > > > there's no guarantee that the close() for each is simultaneous.  If one
> > > > function is released before the other we cannot do a bus reset.  If
> > > > that device is then opened by another user before its sibling is
> > > > released, then we once again cannot perform a link reset.  I don't
> > > > think it would be reasonable to mark the released device quarantined
> > > > until the sibling is released, that would be a terrible user experience.    
> > > 
> > > Not sure why you find it so terrible, and I don't think there's another way.  
> > 
> > If we can't do it without regressing the support we currently have,
> > let's not do it at all.  
> 
> Why would we regress?  As long as there are no unrecoverable errors,
> there's no need to change behaviour at all.

Currently if a fatal error occurs we allow the host to reset the
device, so to the best of our knowledge, the device is always reset.
The proposal here allows gaps where we assume a particular guest
behavior that allows the device to be returned to the host or opened by
other users without that reset.  Any plan that relies on a specific
user behavior is fundamentally wrong imo.

> Alex, do you have a picture of how error recovery can work in your mind?
> Your answers seem to imply you do, and these patches don't implement
> this correctly.  I'm not sure about others, but I for one am unable to
> piece it together from the comments you provide.  If yes, could you
> maybe do a short writeup of an architecture you would be comfortable
> with?

Clearly I have issues with this skip-the-host-reset plan, I don't think
it works.  We cannot assume the user will do error recovery.  As I
stated previously we should enable the user to do error recovery
without depending on the user to do error recovery.  I'm a bit lost why
we're focusing on this approach when the v9 approach of blocking user
access to the device during the host recovery seemed like a better
solution.  I don't think I've said anything bad about that approach,
but it does need further testing and debugging.  Nobody seems to be
interested in debugging why it wasn't quite working to understand
whether that was an implementation issue or a design issue. That's
currently the leading approach from my perspective.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1542257

On Tue, Dec 13, 2016 at 08:00:22PM -0700, Alex Williamson wrote:
> On Wed, 14 Dec 2016 03:58:17 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Dec 13, 2016 at 09:27:59AM -0700, Alex Williamson wrote:
> > > On Tue, 13 Dec 2016 18:12:34 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Mon, Dec 12, 2016 at 08:39:48PM -0700, Alex Williamson wrote:  
> > > > > On Tue, 13 Dec 2016 05:15:13 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >     
> > > > > > On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:    
> > > > > > > > So just don't do it then. Topology must match between host and guest,
> > > > > > > > except maybe for the case of devices with host driver  (e.g. PF)
> > > > > > > > which we might be able to synchronize against.      
> > > > > > > 
> > > > > > > We're talking about host kernel level handling here.  The host kernel
> > > > > > > cannot defer the link reset to the user under the assumption that the
> > > > > > > user is handling the devices in a very specific way.  The moment we do
> > > > > > > that, we've lost.      
> > > > > > 
> > > > > > The way is same as baremetal though, so why not?    
> > > > > 
> > > > > How do we know this?  What if the user is dpdk?  The kernel is
> > > > > responsible for maintaining the integrity of the system and devices,
> > > > > not the user.
> > > > >     
> > > > > > And if user doesn't do what's expected, we can
> > > > > > do the full link reset on close.    
> > > > > 
> > > > > That's exactly my point, if we're talking about multiple devices,
> > > > > there's no guarantee that the close() for each is simultaneous.  If one
> > > > > function is released before the other we cannot do a bus reset.  If
> > > > > that device is then opened by another user before its sibling is
> > > > > released, then we once again cannot perform a link reset.  I don't
> > > > > think it would be reasonable to mark the released device quarantined
> > > > > until the sibling is released, that would be a terrible user experience.    
> > > > 
> > > > Not sure why you find it so terrible, and I don't think there's another way.  
> > > 
> > > If we can't do it without regressing the support we currently have,
> > > let's not do it at all.  
> > 
> > Why would we regress?  As long as there are no unrecoverable errors,
> > there's no need to change behaviour at all.
> 
> Currently if a fatal error occurs we allow the host to reset the
> device, so to the best of our knowledge, the device is always reset.
> The proposal here allows gaps where we assume a particular guest
> behavior that allows the device to be returned to the host or opened by
> other users without that reset.  Any plan that relies on a specific
> user behavior is fundamentally wrong imo.
> 
> > Alex, do you have a picture of how error recovery can work in your mind?
> > Your answers seem to imply you do, and these patches don't implement
> > this correctly.  I'm not sure about others, but I for one am unable to
> > piece it together from the comments you provide.  If yes, could you
> > maybe do a short writeup of an architecture you would be comfortable
> > with?
> 
> Clearly I have issues with this skip-the-host-reset plan, I don't think
> it works.  We cannot assume the user will do error recovery.

Absolutely but we can defer recovery until device close.
Possibly with userspace invoking an ioctl requesting this,
to make sure we don't break any legacy setups.


>  As I
> stated previously we should enable the user to do error recovery
> without depending on the user to do error recovery.  I'm a bit lost why
> we're focusing on this approach when the v9 approach of blocking user
> access to the device during the host recovery seemed like a better
> solution.  I don't think I've said anything bad about that approach,
> but it does need further testing and debugging.  Nobody seems to be
> interested in debugging why it wasn't quite working to understand
> whether that was an implementation issue or a design issue. That's
> currently the leading approach from my perspective.  Thanks,
> 
> Alex

Hmm this doesn't really write up the architecture. It would be
really helpful to have that writeup.

Do I guess right that what v9 tried to do is:
- block future userspace accesses
- do full reset
- re-enable userspace accesses

?

The obvious issue with that is that device loses its state apparently at
random, and this is not something that happens on baremetal, ever.

Thus I don't see how this can work without some PV code in guest.


-- 
MST

[toc] | [prev] | [next] | [standalone]

#1542302

On Thu, 15 Dec 2016 00:20:20 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 13, 2016 at 08:00:22PM -0700, Alex Williamson wrote:
> > On Wed, 14 Dec 2016 03:58:17 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Tue, Dec 13, 2016 at 09:27:59AM -0700, Alex Williamson wrote:  
> > > > On Tue, 13 Dec 2016 18:12:34 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > On Mon, Dec 12, 2016 at 08:39:48PM -0700, Alex Williamson wrote:    
> > > > > > On Tue, 13 Dec 2016 05:15:13 +0200
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > >       
> > > > > > > On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:      
> > > > > > > > > So just don't do it then. Topology must match between host and guest,
> > > > > > > > > except maybe for the case of devices with host driver  (e.g. PF)
> > > > > > > > > which we might be able to synchronize against.        
> > > > > > > > 
> > > > > > > > We're talking about host kernel level handling here.  The host kernel
> > > > > > > > cannot defer the link reset to the user under the assumption that the
> > > > > > > > user is handling the devices in a very specific way.  The moment we do
> > > > > > > > that, we've lost.        
> > > > > > > 
> > > > > > > The way is same as baremetal though, so why not?      
> > > > > > 
> > > > > > How do we know this?  What if the user is dpdk?  The kernel is
> > > > > > responsible for maintaining the integrity of the system and devices,
> > > > > > not the user.
> > > > > >       
> > > > > > > And if user doesn't do what's expected, we can
> > > > > > > do the full link reset on close.      
> > > > > > 
> > > > > > That's exactly my point, if we're talking about multiple devices,
> > > > > > there's no guarantee that the close() for each is simultaneous.  If one
> > > > > > function is released before the other we cannot do a bus reset.  If
> > > > > > that device is then opened by another user before its sibling is
> > > > > > released, then we once again cannot perform a link reset.  I don't
> > > > > > think it would be reasonable to mark the released device quarantined
> > > > > > until the sibling is released, that would be a terrible user experience.      
> > > > > 
> > > > > Not sure why you find it so terrible, and I don't think there's another way.    
> > > > 
> > > > If we can't do it without regressing the support we currently have,
> > > > let's not do it at all.    
> > > 
> > > Why would we regress?  As long as there are no unrecoverable errors,
> > > there's no need to change behaviour at all.  
> > 
> > Currently if a fatal error occurs we allow the host to reset the
> > device, so to the best of our knowledge, the device is always reset.
> > The proposal here allows gaps where we assume a particular guest
> > behavior that allows the device to be returned to the host or opened by
> > other users without that reset.  Any plan that relies on a specific
> > user behavior is fundamentally wrong imo.
> >   
> > > Alex, do you have a picture of how error recovery can work in your mind?
> > > Your answers seem to imply you do, and these patches don't implement
> > > this correctly.  I'm not sure about others, but I for one am unable to
> > > piece it together from the comments you provide.  If yes, could you
> > > maybe do a short writeup of an architecture you would be comfortable
> > > with?  
> > 
> > Clearly I have issues with this skip-the-host-reset plan, I don't think
> > it works.  We cannot assume the user will do error recovery.  
> 
> Absolutely but we can defer recovery until device close.

No we can't, as I've tried to describe multiple times, if the functions
are part of separate groups then they can be opened and closed
asynchronously from each other and we may not have an opportunity where
they are all closed together to perform a reset.  The only option I can
see for this is to quarantine the device, which as I've stated seems
like a really poor solution.

> Possibly with userspace invoking an ioctl requesting this,
> to make sure we don't break any legacy setups.

And how do we know that user is not malicious?  How do we police
whether they actually perform a reset?  We can only track whether a
reset has occurred, which leads back to the quarantine scenario.

> >  As I
> > stated previously we should enable the user to do error recovery
> > without depending on the user to do error recovery.  I'm a bit lost why
> > we're focusing on this approach when the v9 approach of blocking user
> > access to the device during the host recovery seemed like a better
> > solution.  I don't think I've said anything bad about that approach,
> > but it does need further testing and debugging.  Nobody seems to be
> > interested in debugging why it wasn't quite working to understand
> > whether that was an implementation issue or a design issue. That's
> > currently the leading approach from my perspective.  Thanks,
> > 
> > Alex  
> 
> Hmm this doesn't really write up the architecture. It would be
> really helpful to have that writeup.

Perhaps the patch submitters can create one.
 
> Do I guess right that what v9 tried to do is:
> - block future userspace accesses
> - do full reset
> - re-enable userspace accesses
> 
> ?
> 
> The obvious issue with that is that device loses its state apparently at
> random, and this is not something that happens on baremetal, ever.
> 
> Thus I don't see how this can work without some PV code in guest.

So there is no link error that can occur on hardware where the device
becomes inaccessible?  I'm pretty sure I've experienced cases
otherwise.

[toc] | [prev] | [next] | [standalone]

#1542319

On Wed, Dec 14, 2016 at 03:47:43PM -0700, Alex Williamson wrote:
> On Thu, 15 Dec 2016 00:20:20 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Dec 13, 2016 at 08:00:22PM -0700, Alex Williamson wrote:
> > > On Wed, 14 Dec 2016 03:58:17 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Tue, Dec 13, 2016 at 09:27:59AM -0700, Alex Williamson wrote:  
> > > > > On Tue, 13 Dec 2016 18:12:34 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >     
> > > > > > On Mon, Dec 12, 2016 at 08:39:48PM -0700, Alex Williamson wrote:    
> > > > > > > On Tue, 13 Dec 2016 05:15:13 +0200
> > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > >       
> > > > > > > > On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:      
> > > > > > > > > > So just don't do it then. Topology must match between host and guest,
> > > > > > > > > > except maybe for the case of devices with host driver  (e.g. PF)
> > > > > > > > > > which we might be able to synchronize against.        
> > > > > > > > > 
> > > > > > > > > We're talking about host kernel level handling here.  The host kernel
> > > > > > > > > cannot defer the link reset to the user under the assumption that the
> > > > > > > > > user is handling the devices in a very specific way.  The moment we do
> > > > > > > > > that, we've lost.        
> > > > > > > > 
> > > > > > > > The way is same as baremetal though, so why not?      
> > > > > > > 
> > > > > > > How do we know this?  What if the user is dpdk?  The kernel is
> > > > > > > responsible for maintaining the integrity of the system and devices,
> > > > > > > not the user.
> > > > > > >       
> > > > > > > > And if user doesn't do what's expected, we can
> > > > > > > > do the full link reset on close.      
> > > > > > > 
> > > > > > > That's exactly my point, if we're talking about multiple devices,
> > > > > > > there's no guarantee that the close() for each is simultaneous.  If one
> > > > > > > function is released before the other we cannot do a bus reset.  If
> > > > > > > that device is then opened by another user before its sibling is
> > > > > > > released, then we once again cannot perform a link reset.  I don't
> > > > > > > think it would be reasonable to mark the released device quarantined
> > > > > > > until the sibling is released, that would be a terrible user experience.      
> > > > > > 
> > > > > > Not sure why you find it so terrible, and I don't think there's another way.    
> > > > > 
> > > > > If we can't do it without regressing the support we currently have,
> > > > > let's not do it at all.    
> > > > 
> > > > Why would we regress?  As long as there are no unrecoverable errors,
> > > > there's no need to change behaviour at all.  
> > > 
> > > Currently if a fatal error occurs we allow the host to reset the
> > > device, so to the best of our knowledge, the device is always reset.
> > > The proposal here allows gaps where we assume a particular guest
> > > behavior that allows the device to be returned to the host or opened by
> > > other users without that reset.  Any plan that relies on a specific
> > > user behavior is fundamentally wrong imo.
> > >   
> > > > Alex, do you have a picture of how error recovery can work in your mind?
> > > > Your answers seem to imply you do, and these patches don't implement
> > > > this correctly.  I'm not sure about others, but I for one am unable to
> > > > piece it together from the comments you provide.  If yes, could you
> > > > maybe do a short writeup of an architecture you would be comfortable
> > > > with?  
> > > 
> > > Clearly I have issues with this skip-the-host-reset plan, I don't think
> > > it works.  We cannot assume the user will do error recovery.  
> > 
> > Absolutely but we can defer recovery until device close.
> 
> No we can't, as I've tried to describe multiple times, if the functions
> are part of separate groups then they can be opened and closed
> asynchronously from each other and we may not have an opportunity where
> they are all closed together to perform a reset.
> The only option I can
> see for this is to quarantine the device, which as I've stated seems
> like a really poor solution.
> 
> > Possibly with userspace invoking an ioctl requesting this,
> > to make sure we don't break any legacy setups.
> 
> And how do we know that user is not malicious?  How do we police
> whether they actually perform a reset?  We can only track whether a
> reset has occurred, which leads back to the quarantine scenario.

I don't really know what do you call the quarantine scenario.
Malicious users get non working devices.
Why is this poor? 


> > >  As I
> > > stated previously we should enable the user to do error recovery
> > > without depending on the user to do error recovery.  I'm a bit lost why
> > > we're focusing on this approach when the v9 approach of blocking user
> > > access to the device during the host recovery seemed like a better
> > > solution.  I don't think I've said anything bad about that approach,
> > > but it does need further testing and debugging.  Nobody seems to be
> > > interested in debugging why it wasn't quite working to understand
> > > whether that was an implementation issue or a design issue. That's
> > > currently the leading approach from my perspective.  Thanks,
> > > 
> > > Alex  
> > 
> > Hmm this doesn't really write up the architecture. It would be
> > really helpful to have that writeup.
> 
> Perhaps the patch submitters can create one.

You already spent so many words on this - won't it
be easier for you to just write up what you consider
the right thing to do?


> > Do I guess right that what v9 tried to do is:
> > - block future userspace accesses
> > - do full reset
> > - re-enable userspace accesses
> > 
> > ?
> > 
> > The obvious issue with that is that device loses its state apparently at
> > random, and this is not something that happens on baremetal, ever.
> > 
> > Thus I don't see how this can work without some PV code in guest.
> 
> So there is no link error that can occur on hardware where the device
> becomes inaccessible?  I'm pretty sure I've experienced cases
> otherwise.

There are errors like this (losing link) but this is IMO a major error
which isn't covered by AER.

It's possible that some guests have AER code that happens to recover
from lost link, but IMO that's never guaranteed.

Converting an uncorrectable error like a lost write into link loss
means escalating the problem to a whole different level.


-- 
MST

[toc] | [prev] | [next] | [standalone]

#1542342

On Thu, 15 Dec 2016 01:00:06 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Dec 14, 2016 at 03:47:43PM -0700, Alex Williamson wrote:
> > On Thu, 15 Dec 2016 00:20:20 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Tue, Dec 13, 2016 at 08:00:22PM -0700, Alex Williamson wrote:  
> > > > On Wed, 14 Dec 2016 03:58:17 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > On Tue, Dec 13, 2016 at 09:27:59AM -0700, Alex Williamson wrote:    
> > > > > > On Tue, 13 Dec 2016 18:12:34 +0200
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > >       
> > > > > > > On Mon, Dec 12, 2016 at 08:39:48PM -0700, Alex Williamson wrote:      
> > > > > > > > On Tue, 13 Dec 2016 05:15:13 +0200
> > > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > >         
> > > > > > > > > On Mon, Dec 12, 2016 at 03:43:13PM -0700, Alex Williamson wrote:        
> > > > > > > > > > > So just don't do it then. Topology must match between host and guest,
> > > > > > > > > > > except maybe for the case of devices with host driver  (e.g. PF)
> > > > > > > > > > > which we might be able to synchronize against.          
> > > > > > > > > > 
> > > > > > > > > > We're talking about host kernel level handling here.  The host kernel
> > > > > > > > > > cannot defer the link reset to the user under the assumption that the
> > > > > > > > > > user is handling the devices in a very specific way.  The moment we do
> > > > > > > > > > that, we've lost.          
> > > > > > > > > 
> > > > > > > > > The way is same as baremetal though, so why not?        
> > > > > > > > 
> > > > > > > > How do we know this?  What if the user is dpdk?  The kernel is
> > > > > > > > responsible for maintaining the integrity of the system and devices,
> > > > > > > > not the user.
> > > > > > > >         
> > > > > > > > > And if user doesn't do what's expected, we can
> > > > > > > > > do the full link reset on close.        
> > > > > > > > 
> > > > > > > > That's exactly my point, if we're talking about multiple devices,
> > > > > > > > there's no guarantee that the close() for each is simultaneous.  If one
> > > > > > > > function is released before the other we cannot do a bus reset.  If
> > > > > > > > that device is then opened by another user before its sibling is
> > > > > > > > released, then we once again cannot perform a link reset.  I don't
> > > > > > > > think it would be reasonable to mark the released device quarantined
> > > > > > > > until the sibling is released, that would be a terrible user experience.        
> > > > > > > 
> > > > > > > Not sure why you find it so terrible, and I don't think there's another way.      
> > > > > > 
> > > > > > If we can't do it without regressing the support we currently have,
> > > > > > let's not do it at all.      
> > > > > 
> > > > > Why would we regress?  As long as there are no unrecoverable errors,
> > > > > there's no need to change behaviour at all.    
> > > > 
> > > > Currently if a fatal error occurs we allow the host to reset the
> > > > device, so to the best of our knowledge, the device is always reset.
> > > > The proposal here allows gaps where we assume a particular guest
> > > > behavior that allows the device to be returned to the host or opened by
> > > > other users without that reset.  Any plan that relies on a specific
> > > > user behavior is fundamentally wrong imo.
> > > >     
> > > > > Alex, do you have a picture of how error recovery can work in your mind?
> > > > > Your answers seem to imply you do, and these patches don't implement
> > > > > this correctly.  I'm not sure about others, but I for one am unable to
> > > > > piece it together from the comments you provide.  If yes, could you
> > > > > maybe do a short writeup of an architecture you would be comfortable
> > > > > with?    
> > > > 
> > > > Clearly I have issues with this skip-the-host-reset plan, I don't think
> > > > it works.  We cannot assume the user will do error recovery.    
> > > 
> > > Absolutely but we can defer recovery until device close.  
> > 
> > No we can't, as I've tried to describe multiple times, if the functions
> > are part of separate groups then they can be opened and closed
> > asynchronously from each other and we may not have an opportunity where
> > they are all closed together to perform a reset.
> > The only option I can
> > see for this is to quarantine the device, which as I've stated seems
> > like a really poor solution.
> >   
> > > Possibly with userspace invoking an ioctl requesting this,
> > > to make sure we don't break any legacy setups.  
> > 
> > And how do we know that user is not malicious?  How do we police
> > whether they actually perform a reset?  We can only track whether a
> > reset has occurred, which leads back to the quarantine scenario.  
> 
> I don't really know what do you call the quarantine scenario.
> Malicious users get non working devices.
> Why is this poor? 

If we were to skip the host link reset portion of recovery, we'd need
to mark the devices that would have been reset with some sort of dirty
flag.  A user performing a link reset affecting the device would clear
the dirty flag.  If a group is released by the user with devices having
the flag set, the group would be quarantined and we'd need to prevent
the devices within it from being released to the host or the group
being opened by another user.  vfio-pci would need to actively try to
reset the group whenever another group is released.

It's not a malicious user shooting themselves, it's a malicious user
causing a DoS by leaving a device in a tainted state where we cannot
reuse it.  It also inches us ever further up the complexity scale for
this feature.

> > > >  As I
> > > > stated previously we should enable the user to do error recovery
> > > > without depending on the user to do error recovery.  I'm a bit lost why
> > > > we're focusing on this approach when the v9 approach of blocking user
> > > > access to the device during the host recovery seemed like a better
> > > > solution.  I don't think I've said anything bad about that approach,
> > > > but it does need further testing and debugging.  Nobody seems to be
> > > > interested in debugging why it wasn't quite working to understand
> > > > whether that was an implementation issue or a design issue. That's
> > > > currently the leading approach from my perspective.  Thanks,
> > > > 
> > > > Alex    
> > > 
> > > Hmm this doesn't really write up the architecture. It would be
> > > really helpful to have that writeup.  
> > 
> > Perhaps the patch submitters can create one.  
> 
> You already spent so many words on this - won't it
> be easier for you to just write up what you consider
> the right thing to do?
>
> > > Do I guess right that what v9 tried to do is:
> > > - block future userspace accesses
> > > - do full reset
> > > - re-enable userspace accesses
> > > 
> > > ?
> > > 
> > > The obvious issue with that is that device loses its state apparently at
> > > random, and this is not something that happens on baremetal, ever.
> > > 
> > > Thus I don't see how this can work without some PV code in guest.  
> > 
> > So there is no link error that can occur on hardware where the device
> > becomes inaccessible?  I'm pretty sure I've experienced cases
> > otherwise.  
> 
> There are errors like this (losing link) but this is IMO a major error
> which isn't covered by AER.
> 
> It's possible that some guests have AER code that happens to recover
> from lost link, but IMO that's never guaranteed.
> 
> Converting an uncorrectable error like a lost write into link loss
> means escalating the problem to a whole different level.

If a host induced link reset cannot work, then we're at an impasse and
I have no recommended architecture to write-up.

[toc] | [prev] | [next] | [standalone]

#1541828

Sorry for late.
after reading all your comments, I think I will try the solution 1.

On 12/13/2016 03:12 AM, Alex Williamson wrote:
> On Mon, 12 Dec 2016 21:49:01 +0800
> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> 
>> Hi,
>> I have 2 solutions(high level design) came to me, please see if they are
>> acceptable, or which one is acceptable. Also have some questions.
>>
>> 1. block guest access during host recovery
>>
>>    add new field error_recovering in struct vfio_pci_device to
>>    indicate host recovery status. aer driver in host will still do
>>    reset link
>>
>>    - set error_recovering in vfio-pci driver's error_detected, used to
>>      block all kinds of user access(config space, mmio)
>>    - in order to solve concurrent issue of device resetting & user
>>      access, check device state[*] in vfio-pci driver's resume, see if
>>      device reset is done, if it is, then clear"error_recovering", or
>>      else new a timer, check device state periodically until device
>>      reset is done. (what if device reset don't end for a long time?)
>>    - In qemu, translate guest link reset to host link reset.
>>      A question here: we already have link reset in host, is a second
>>      link reset necessary? why?
>>  
>>    [*] how to check device state: reading certain config space
>>        register, check return value is valid or not(All F's)
> 
> Isn't this exactly the path we were on previously?

Yes, it is basically the previous path, plus the optimization.

> There might be an
> optimization that we could skip back-to-back resets, but how can you
> necessarily infer that the resets are for the same thing? If the user
> accesses the device between resets, can you still guarantee the guest
> directed reset is unnecessary?  If time passes between resets, do you
> know they're for the same event?  How much time can pass between the
> host and guest reset to know they're for the same event?  In the
> process of error handling, which is more important, speed or
> correctness?
>  

I think vfio driver itself won't know what each reset comes for, and I
don't quite understand why should vfio care this question, is this a new
question in the design?

But I think it make sense that the user access during 2 resets maybe a
trouble for guest recovery, misbehaved user could be out of our
imagination.  Correctness is more important.

If I understand you right, let me make a summary: host recovery just
does link reset, which is incomplete, so we'd better do a complete guest
recovery for correctness.

>> 2. skip link reset in aer driver of host kernel, for vfio-pci.
>>    Let user decide how to do serious recovery
>>
>>    add new field "user_driver" in struct pci_dev, used to skip link
>>    reset for vfio-pci; add new field "link_reset" in struct
>>    vfio_pci_device to indicate link has been reset or not during
>>    recovery
>>
>>    - set user_driver in vfio_pci_probe(), to skip link reset for
>>      vfio-pci in host.
>>    - (use a flag)block user access(config, mmio) during host recovery
>>      (not sure if this step is necessary)
>>    - In qemu, translate guest link reset to host link reset.
>>    - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
>>      is executed
>>    - In vfio-pci driver's resume, new a timer, check "link_reset" field
>>      periodically, if it is set in reasonable time, then clear it and
>>      delete timer, or else, vfio-pci driver will does the link reset!
> 
> What happens in the case of a multifunction device where each function
> is part of a separate IOMMU group and one function is hot-removed from
> the user? We can't do a link reset on that function since the other
> function is still in use.  We have no choice but release a device in an
> unknown state back to the host.

hot-remove from user, do you mean, for example, all functions assigned
to VM, then suddenly a person does something like following

$ echo 0000:06:00.0 > /sys/bus/pci/drivers/vfio-pci/unbind

$ echo 0000:06:00.0 > /sys/bus/pci/drivers/igb/bind

to return device to host driver, or don't bind it to host driver, let it
in driver-less state???

>  As previously discussed, we don't
> expect that any sort of function-level FLR will necessarily reset the
> device to the same state.  I also don't really like vfio-pci taking
> over error handling capabilities from the PCI-core.  That's redundant
> code and extra maintenance overhead.
>  

I understand the concern, so I suppose solution 1 is preferred.

-- 
Sincerely,
Cao jin

>> A quick question:
>> I don't know how devices is divided into iommu groups, is it possible
>> for functions in a multi-function device to be split into different groups?
> 
> Yes, if a multifunction device supports ACS or if we have quirks to
> expose that the functions do not perform internal peer-to-peer, then
> they may be in separate IOMMU groups, depending on the rest of the PCI
> topology.  See:
> 
> http://vfio.blogspot.com/2014/08/iommu-groups-inside-and-out.html
> 
> Thanks,
> Alex
> 
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1542246

On Wed, 14 Dec 2016 18:24:23 +0800
Cao jin <caoj.fnst@cn.fujitsu.com> wrote:

> Sorry for late.
> after reading all your comments, I think I will try the solution 1.
> 
> On 12/13/2016 03:12 AM, Alex Williamson wrote:
> > On Mon, 12 Dec 2016 21:49:01 +0800
> > Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> >   
> >> Hi,
> >> I have 2 solutions(high level design) came to me, please see if they are
> >> acceptable, or which one is acceptable. Also have some questions.
> >>
> >> 1. block guest access during host recovery
> >>
> >>    add new field error_recovering in struct vfio_pci_device to
> >>    indicate host recovery status. aer driver in host will still do
> >>    reset link
> >>
> >>    - set error_recovering in vfio-pci driver's error_detected, used to
> >>      block all kinds of user access(config space, mmio)
> >>    - in order to solve concurrent issue of device resetting & user
> >>      access, check device state[*] in vfio-pci driver's resume, see if
> >>      device reset is done, if it is, then clear"error_recovering", or
> >>      else new a timer, check device state periodically until device
> >>      reset is done. (what if device reset don't end for a long time?)
> >>    - In qemu, translate guest link reset to host link reset.
> >>      A question here: we already have link reset in host, is a second
> >>      link reset necessary? why?
> >>  
> >>    [*] how to check device state: reading certain config space
> >>        register, check return value is valid or not(All F's)  
> > 
> > Isn't this exactly the path we were on previously?  
> 
> Yes, it is basically the previous path, plus the optimization.
> 
> > There might be an
> > optimization that we could skip back-to-back resets, but how can you
> > necessarily infer that the resets are for the same thing? If the user
> > accesses the device between resets, can you still guarantee the guest
> > directed reset is unnecessary?  If time passes between resets, do you
> > know they're for the same event?  How much time can pass between the
> > host and guest reset to know they're for the same event?  In the
> > process of error handling, which is more important, speed or
> > correctness?
> >    
> 
> I think vfio driver itself won't know what each reset comes for, and I
> don't quite understand why should vfio care this question, is this a new
> question in the design?

You're suggesting an optimization to eliminate one of the resets,
and as we've discussed, I don't see removing the host induced reset
as a viable option.  That means you want to eliminate the guest
directed reset.  There are potentially three levels to do that, the
vfio-pci driver in the host kernel, somewhere in QEMU, or eliminate it
within the guest.  My comments were directed to the first option, the
host kernel level cannot correlate user directed resets as duplicates
of host directed resets.  
 
> But I think it make sense that the user access during 2 resets maybe a
> trouble for guest recovery, misbehaved user could be out of our
> imagination.  Correctness is more important.
> 
> If I understand you right, let me make a summary: host recovery just
> does link reset, which is incomplete, so we'd better do a complete guest
> recovery for correctness.

We don't know whether the host link reset is incomplete, but we can't do
a link reset transparently to the device, the device is no longer in the
same state after the reset.  The device specific driver, which exists
in userspace needs to be involved in device recovery.  Therefore
regardless of how QEMU handles the error, the driver within the guest
needs to be notified and perform recovery.  Since the device is PCI and
we're on x86 and nobody wants to introduce paravirtual error recovery,
we must use AER.  Part of AER recovery includes the possibility of
performing a link reset.  So it seems this eliminates avoiding the link
reset within the guest.

That leaves QEMU.  Here we need to decide whether a guest triggered
link reset induces a host link reset.  The current working theory is
that yes, this must be the case.  If there is ever a case where a
driver within the guest could trigger a link reset for the purposes
of error recovery when the host has not, I think this must be the
case.  Therefore, at least some guest induced link resets must become
host link resets.  Currently we assume all guest induced link resets
become host link resets.  Minimally to avoid that, QEMU would need to
know (not assume) whether the host performed a link reset.  Even with
that, QEMU would need to be able to correlate that a link reset from
the guest is a duplicate of a link reset that was already performed by
the host.  That implies that QEMU needs to deduce the intention of
the guest.  That seems like a complicated task for a patch series that
is already complicated enough, especially for a feature of questionable
value given the configuration restrictions (imo).

I would much rather focus on getting it right and making it as simple
as we can, even if that means links get reset one too many times on
error.

> >> 2. skip link reset in aer driver of host kernel, for vfio-pci.
> >>    Let user decide how to do serious recovery
> >>
> >>    add new field "user_driver" in struct pci_dev, used to skip link
> >>    reset for vfio-pci; add new field "link_reset" in struct
> >>    vfio_pci_device to indicate link has been reset or not during
> >>    recovery
> >>
> >>    - set user_driver in vfio_pci_probe(), to skip link reset for
> >>      vfio-pci in host.
> >>    - (use a flag)block user access(config, mmio) during host recovery
> >>      (not sure if this step is necessary)
> >>    - In qemu, translate guest link reset to host link reset.
> >>    - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
> >>      is executed
> >>    - In vfio-pci driver's resume, new a timer, check "link_reset" field
> >>      periodically, if it is set in reasonable time, then clear it and
> >>      delete timer, or else, vfio-pci driver will does the link reset!  
> > 
> > What happens in the case of a multifunction device where each function
> > is part of a separate IOMMU group and one function is hot-removed from
> > the user? We can't do a link reset on that function since the other
> > function is still in use.  We have no choice but release a device in an
> > unknown state back to the host.  
> 
> hot-remove from user, do you mean, for example, all functions assigned
> to VM, then suddenly a person does something like following
> 
> $ echo 0000:06:00.0 > /sys/bus/pci/drivers/vfio-pci/unbind
> 
> $ echo 0000:06:00.0 > /sys/bus/pci/drivers/igb/bind
> 
> to return device to host driver, or don't bind it to host driver, let it
> in driver-less state???

Yes, the host kernel has no visiblity to how a user is making use of
devices.  To support AER we require a similar topology between host and
guest such that a guest link reset translates to a host reset.  That
requirement is imposed by userspace, ie. QEMU.  The host kernel cannot
presume that this is the case.  Therefore we could have a
multi-function device where each function is assigned to the same or
different users in any configuration.  If a fault occurs and we defer
to the user to perform the link reset, we have absolutely no guarantee
that it will ever occur.  If the functions are assigned to different
users, then each user individually doesn't have the capability to
perform a link reset.  If the devices happen to be assigned to a single
user when the error occurs, we cannot assume the user has an AER
compatible configuration, the devices could be exposed as separate
single function devices, any one of which might be individually removed
from the user and made use of by the host, such as your sysfs example
above.  The host cannot perform a link reset in this case either
as the sibling devices are still in use by the guest.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1542259

On Wed, Dec 14, 2016 at 03:16:37PM -0700, Alex Williamson wrote:
> On Wed, 14 Dec 2016 18:24:23 +0800
> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> 
> > Sorry for late.
> > after reading all your comments, I think I will try the solution 1.
> > 
> > On 12/13/2016 03:12 AM, Alex Williamson wrote:
> > > On Mon, 12 Dec 2016 21:49:01 +0800
> > > Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> > >   
> > >> Hi,
> > >> I have 2 solutions(high level design) came to me, please see if they are
> > >> acceptable, or which one is acceptable. Also have some questions.
> > >>
> > >> 1. block guest access during host recovery
> > >>
> > >>    add new field error_recovering in struct vfio_pci_device to
> > >>    indicate host recovery status. aer driver in host will still do
> > >>    reset link
> > >>
> > >>    - set error_recovering in vfio-pci driver's error_detected, used to
> > >>      block all kinds of user access(config space, mmio)
> > >>    - in order to solve concurrent issue of device resetting & user
> > >>      access, check device state[*] in vfio-pci driver's resume, see if
> > >>      device reset is done, if it is, then clear"error_recovering", or
> > >>      else new a timer, check device state periodically until device
> > >>      reset is done. (what if device reset don't end for a long time?)
> > >>    - In qemu, translate guest link reset to host link reset.
> > >>      A question here: we already have link reset in host, is a second
> > >>      link reset necessary? why?
> > >>  
> > >>    [*] how to check device state: reading certain config space
> > >>        register, check return value is valid or not(All F's)  
> > > 
> > > Isn't this exactly the path we were on previously?  
> > 
> > Yes, it is basically the previous path, plus the optimization.
> > 
> > > There might be an
> > > optimization that we could skip back-to-back resets, but how can you
> > > necessarily infer that the resets are for the same thing? If the user
> > > accesses the device between resets, can you still guarantee the guest
> > > directed reset is unnecessary?  If time passes between resets, do you
> > > know they're for the same event?  How much time can pass between the
> > > host and guest reset to know they're for the same event?  In the
> > > process of error handling, which is more important, speed or
> > > correctness?
> > >    
> > 
> > I think vfio driver itself won't know what each reset comes for, and I
> > don't quite understand why should vfio care this question, is this a new
> > question in the design?
> 
> You're suggesting an optimization to eliminate one of the resets,
> and as we've discussed, I don't see removing the host induced reset
> as a viable option.  That means you want to eliminate the guest
> directed reset.  There are potentially three levels to do that, the
> vfio-pci driver in the host kernel, somewhere in QEMU, or eliminate it
> within the guest.  My comments were directed to the first option, the
> host kernel level cannot correlate user directed resets as duplicates
> of host directed resets.  
>  
> > But I think it make sense that the user access during 2 resets maybe a
> > trouble for guest recovery, misbehaved user could be out of our
> > imagination.  Correctness is more important.
> > 
> > If I understand you right, let me make a summary: host recovery just
> > does link reset, which is incomplete, so we'd better do a complete guest
> > recovery for correctness.
> 
> We don't know whether the host link reset is incomplete, but we can't do
> a link reset transparently to the device, the device is no longer in the
> same state after the reset.  The device specific driver, which exists
> in userspace needs to be involved in device recovery.  Therefore
> regardless of how QEMU handles the error, the driver within the guest
> needs to be notified and perform recovery.  Since the device is PCI and
> we're on x86 and nobody wants to introduce paravirtual error recovery,
> we must use AER.  Part of AER recovery includes the possibility of
> performing a link reset.  So it seems this eliminates avoiding the link
> reset within the guest.
> 
> That leaves QEMU.  Here we need to decide whether a guest triggered
> link reset induces a host link reset.  The current working theory is
> that yes, this must be the case.  If there is ever a case where a
> driver within the guest could trigger a link reset for the purposes
> of error recovery when the host has not, I think this must be the
> case.  Therefore, at least some guest induced link resets must become
> host link resets.  Currently we assume all guest induced link resets
> become host link resets.  Minimally to avoid that, QEMU would need to
> know (not assume) whether the host performed a link reset.  Even with
> that, QEMU would need to be able to correlate that a link reset from
> the guest is a duplicate of a link reset that was already performed by
> the host.  That implies that QEMU needs to deduce the intention of
> the guest.  That seems like a complicated task for a patch series that
> is already complicated enough, especially for a feature of questionable
> value given the configuration restrictions (imo).
> 
> I would much rather focus on getting it right and making it as simple
> as we can, even if that means links get reset one too many times on
> error.
> 
> > >> 2. skip link reset in aer driver of host kernel, for vfio-pci.
> > >>    Let user decide how to do serious recovery
> > >>
> > >>    add new field "user_driver" in struct pci_dev, used to skip link
> > >>    reset for vfio-pci; add new field "link_reset" in struct
> > >>    vfio_pci_device to indicate link has been reset or not during
> > >>    recovery
> > >>
> > >>    - set user_driver in vfio_pci_probe(), to skip link reset for
> > >>      vfio-pci in host.
> > >>    - (use a flag)block user access(config, mmio) during host recovery
> > >>      (not sure if this step is necessary)
> > >>    - In qemu, translate guest link reset to host link reset.
> > >>    - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
> > >>      is executed
> > >>    - In vfio-pci driver's resume, new a timer, check "link_reset" field
> > >>      periodically, if it is set in reasonable time, then clear it and
> > >>      delete timer, or else, vfio-pci driver will does the link reset!  
> > > 
> > > What happens in the case of a multifunction device where each function
> > > is part of a separate IOMMU group and one function is hot-removed from
> > > the user? We can't do a link reset on that function since the other
> > > function is still in use.  We have no choice but release a device in an
> > > unknown state back to the host.  
> > 
> > hot-remove from user, do you mean, for example, all functions assigned
> > to VM, then suddenly a person does something like following
> > 
> > $ echo 0000:06:00.0 > /sys/bus/pci/drivers/vfio-pci/unbind
> > 
> > $ echo 0000:06:00.0 > /sys/bus/pci/drivers/igb/bind
> > 
> > to return device to host driver, or don't bind it to host driver, let it
> > in driver-less state???
> 
> Yes, the host kernel has no visiblity to how a user is making use of
> devices.  To support AER we require a similar topology between host and
> guest such that a guest link reset translates to a host reset.  That
> requirement is imposed by userspace, ie. QEMU.  The host kernel cannot
> presume that this is the case.

So enforce this to enable recovery functionality. Why can't you?

>  Therefore we could have a
> multi-function device where each function is assigned to the same or
> different users in any configuration.  If a fault occurs and we defer
> to the user to perform the link reset, we have absolutely no guarantee
> that it will ever occur.  If the functions are assigned to different
> users, then each user individually doesn't have the capability to
> perform a link reset.  If the devices happen to be assigned to a single
> user when the error occurs, we cannot assume the user has an AER
> compatible configuration, the devices could be exposed as separate
> single function devices, any one of which might be individually removed
> from the user and made use of by the host, such as your sysfs example
> above.  The host cannot perform a link reset in this case either
> as the sibling devices are still in use by the guest.  Thanks,
> 
> Alex

[toc] | [prev] | [next] | [standalone]

#1542318

On Thu, 15 Dec 2016 00:25:13 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Dec 14, 2016 at 03:16:37PM -0700, Alex Williamson wrote:
> > On Wed, 14 Dec 2016 18:24:23 +0800
> > Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> >   
> > > Sorry for late.
> > > after reading all your comments, I think I will try the solution 1.
> > > 
> > > On 12/13/2016 03:12 AM, Alex Williamson wrote:  
> > > > On Mon, 12 Dec 2016 21:49:01 +0800
> > > > Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> > > >     
> > > >> Hi,
> > > >> I have 2 solutions(high level design) came to me, please see if they are
> > > >> acceptable, or which one is acceptable. Also have some questions.
> > > >>
> > > >> 1. block guest access during host recovery
> > > >>
> > > >>    add new field error_recovering in struct vfio_pci_device to
> > > >>    indicate host recovery status. aer driver in host will still do
> > > >>    reset link
> > > >>
> > > >>    - set error_recovering in vfio-pci driver's error_detected, used to
> > > >>      block all kinds of user access(config space, mmio)
> > > >>    - in order to solve concurrent issue of device resetting & user
> > > >>      access, check device state[*] in vfio-pci driver's resume, see if
> > > >>      device reset is done, if it is, then clear"error_recovering", or
> > > >>      else new a timer, check device state periodically until device
> > > >>      reset is done. (what if device reset don't end for a long time?)
> > > >>    - In qemu, translate guest link reset to host link reset.
> > > >>      A question here: we already have link reset in host, is a second
> > > >>      link reset necessary? why?
> > > >>  
> > > >>    [*] how to check device state: reading certain config space
> > > >>        register, check return value is valid or not(All F's)    
> > > > 
> > > > Isn't this exactly the path we were on previously?    
> > > 
> > > Yes, it is basically the previous path, plus the optimization.
> > >   
> > > > There might be an
> > > > optimization that we could skip back-to-back resets, but how can you
> > > > necessarily infer that the resets are for the same thing? If the user
> > > > accesses the device between resets, can you still guarantee the guest
> > > > directed reset is unnecessary?  If time passes between resets, do you
> > > > know they're for the same event?  How much time can pass between the
> > > > host and guest reset to know they're for the same event?  In the
> > > > process of error handling, which is more important, speed or
> > > > correctness?
> > > >      
> > > 
> > > I think vfio driver itself won't know what each reset comes for, and I
> > > don't quite understand why should vfio care this question, is this a new
> > > question in the design?  
> > 
> > You're suggesting an optimization to eliminate one of the resets,
> > and as we've discussed, I don't see removing the host induced reset
> > as a viable option.  That means you want to eliminate the guest
> > directed reset.  There are potentially three levels to do that, the
> > vfio-pci driver in the host kernel, somewhere in QEMU, or eliminate it
> > within the guest.  My comments were directed to the first option, the
> > host kernel level cannot correlate user directed resets as duplicates
> > of host directed resets.  
> >    
> > > But I think it make sense that the user access during 2 resets maybe a
> > > trouble for guest recovery, misbehaved user could be out of our
> > > imagination.  Correctness is more important.
> > > 
> > > If I understand you right, let me make a summary: host recovery just
> > > does link reset, which is incomplete, so we'd better do a complete guest
> > > recovery for correctness.  
> > 
> > We don't know whether the host link reset is incomplete, but we can't do
> > a link reset transparently to the device, the device is no longer in the
> > same state after the reset.  The device specific driver, which exists
> > in userspace needs to be involved in device recovery.  Therefore
> > regardless of how QEMU handles the error, the driver within the guest
> > needs to be notified and perform recovery.  Since the device is PCI and
> > we're on x86 and nobody wants to introduce paravirtual error recovery,
> > we must use AER.  Part of AER recovery includes the possibility of
> > performing a link reset.  So it seems this eliminates avoiding the link
> > reset within the guest.
> > 
> > That leaves QEMU.  Here we need to decide whether a guest triggered
> > link reset induces a host link reset.  The current working theory is
> > that yes, this must be the case.  If there is ever a case where a
> > driver within the guest could trigger a link reset for the purposes
> > of error recovery when the host has not, I think this must be the
> > case.  Therefore, at least some guest induced link resets must become
> > host link resets.  Currently we assume all guest induced link resets
> > become host link resets.  Minimally to avoid that, QEMU would need to
> > know (not assume) whether the host performed a link reset.  Even with
> > that, QEMU would need to be able to correlate that a link reset from
> > the guest is a duplicate of a link reset that was already performed by
> > the host.  That implies that QEMU needs to deduce the intention of
> > the guest.  That seems like a complicated task for a patch series that
> > is already complicated enough, especially for a feature of questionable
> > value given the configuration restrictions (imo).
> > 
> > I would much rather focus on getting it right and making it as simple
> > as we can, even if that means links get reset one too many times on
> > error.
> >   
> > > >> 2. skip link reset in aer driver of host kernel, for vfio-pci.
> > > >>    Let user decide how to do serious recovery
> > > >>
> > > >>    add new field "user_driver" in struct pci_dev, used to skip link
> > > >>    reset for vfio-pci; add new field "link_reset" in struct
> > > >>    vfio_pci_device to indicate link has been reset or not during
> > > >>    recovery
> > > >>
> > > >>    - set user_driver in vfio_pci_probe(), to skip link reset for
> > > >>      vfio-pci in host.
> > > >>    - (use a flag)block user access(config, mmio) during host recovery
> > > >>      (not sure if this step is necessary)
> > > >>    - In qemu, translate guest link reset to host link reset.
> > > >>    - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
> > > >>      is executed
> > > >>    - In vfio-pci driver's resume, new a timer, check "link_reset" field
> > > >>      periodically, if it is set in reasonable time, then clear it and
> > > >>      delete timer, or else, vfio-pci driver will does the link reset!    
> > > > 
> > > > What happens in the case of a multifunction device where each function
> > > > is part of a separate IOMMU group and one function is hot-removed from
> > > > the user? We can't do a link reset on that function since the other
> > > > function is still in use.  We have no choice but release a device in an
> > > > unknown state back to the host.    
> > > 
> > > hot-remove from user, do you mean, for example, all functions assigned
> > > to VM, then suddenly a person does something like following
> > > 
> > > $ echo 0000:06:00.0 > /sys/bus/pci/drivers/vfio-pci/unbind
> > > 
> > > $ echo 0000:06:00.0 > /sys/bus/pci/drivers/igb/bind
> > > 
> > > to return device to host driver, or don't bind it to host driver, let it
> > > in driver-less state???  
> > 
> > Yes, the host kernel has no visiblity to how a user is making use of
> > devices.  To support AER we require a similar topology between host and
> > guest such that a guest link reset translates to a host reset.  That
> > requirement is imposed by userspace, ie. QEMU.  The host kernel cannot
> > presume that this is the case.  
> 
> So enforce this to enable recovery functionality. Why can't you?

How?
 
> >  Therefore we could have a
> > multi-function device where each function is assigned to the same or
> > different users in any configuration.  If a fault occurs and we defer
> > to the user to perform the link reset, we have absolutely no guarantee
> > that it will ever occur.  If the functions are assigned to different
> > users, then each user individually doesn't have the capability to
> > perform a link reset.  If the devices happen to be assigned to a single
> > user when the error occurs, we cannot assume the user has an AER
> > compatible configuration, the devices could be exposed as separate
> > single function devices, any one of which might be individually removed
> > from the user and made use of by the host, such as your sysfs example
> > above.  The host cannot perform a link reset in this case either
> > as the sibling devices are still in use by the guest.  Thanks,
> > 
> > Alex  

[toc] | [prev] | [next] | [standalone]

#1542742

On 12/15/2016 06:16 AM, Alex Williamson wrote:
> On Wed, 14 Dec 2016 18:24:23 +0800
> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> 
>> Sorry for late.
>> after reading all your comments, I think I will try the solution 1.
>>
>> On 12/13/2016 03:12 AM, Alex Williamson wrote:
>>> On Mon, 12 Dec 2016 21:49:01 +0800
>>> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>>>   
>>>> Hi,
>>>> I have 2 solutions(high level design) came to me, please see if they are
>>>> acceptable, or which one is acceptable. Also have some questions.
>>>>
>>>> 1. block guest access during host recovery
>>>>
>>>>    add new field error_recovering in struct vfio_pci_device to
>>>>    indicate host recovery status. aer driver in host will still do
>>>>    reset link
>>>>
>>>>    - set error_recovering in vfio-pci driver's error_detected, used to
>>>>      block all kinds of user access(config space, mmio)
>>>>    - in order to solve concurrent issue of device resetting & user
>>>>      access, check device state[*] in vfio-pci driver's resume, see if
>>>>      device reset is done, if it is, then clear"error_recovering", or
>>>>      else new a timer, check device state periodically until device
>>>>      reset is done. (what if device reset don't end for a long time?)
>>>>    - In qemu, translate guest link reset to host link reset.
>>>>      A question here: we already have link reset in host, is a second
>>>>      link reset necessary? why?
>>>>  
>>>>    [*] how to check device state: reading certain config space
>>>>        register, check return value is valid or not(All F's)  
>>>
>>> Isn't this exactly the path we were on previously?  
>>
>> Yes, it is basically the previous path, plus the optimization.
>>
>>> There might be an
>>> optimization that we could skip back-to-back resets, but how can you
>>> necessarily infer that the resets are for the same thing? If the user
>>> accesses the device between resets, can you still guarantee the guest
>>> directed reset is unnecessary?  If time passes between resets, do you
>>> know they're for the same event?  How much time can pass between the
>>> host and guest reset to know they're for the same event?  In the
>>> process of error handling, which is more important, speed or
>>> correctness?
>>>    
>>
>> I think vfio driver itself won't know what each reset comes for, and I
>> don't quite understand why should vfio care this question, is this a new
>> question in the design?
> 
> You're suggesting an optimization to eliminate one of the resets,
> and as we've discussed, I don't see removing the host induced reset
> as a viable option.  That means you want to eliminate the guest
> directed reset.  There are potentially three levels to do that, the
> vfio-pci driver in the host kernel, somewhere in QEMU, or eliminate it
> within the guest.  My comments were directed to the first option, the
> host kernel level cannot correlate user directed resets as duplicates
> of host directed resets.  
>  

Ah, maybe it is mistake, I don't really want to eliminate guest directed
reset very much, I was just not sure why it is very necessary.

The optimization I said just is fully separating host recovery from
guest recovery(timer, check device periodically) in time, because there
is concurrent device resetting & user access.

>> But I think it make sense that the user access during 2 resets maybe a
>> trouble for guest recovery, misbehaved user could be out of our
>> imagination.  Correctness is more important.
>>
>> If I understand you right, let me make a summary: host recovery just
>> does link reset, which is incomplete, so we'd better do a complete guest
>> recovery for correctness.
> 
> We don't know whether the host link reset is incomplete, but we can't do
> a link reset transparently to the device, the device is no longer in the
> same state after the reset.  The device specific driver, which exists
> in userspace needs to be involved in device recovery.  Therefore
> regardless of how QEMU handles the error, the driver within the guest
> needs to be notified and perform recovery.  Since the device is PCI and
> we're on x86 and nobody wants to introduce paravirtual error recovery,
> we must use AER.  Part of AER recovery includes the possibility of
> performing a link reset.  So it seems this eliminates avoiding the link
> reset within the guest.
> 
> That leaves QEMU.  Here we need to decide whether a guest triggered
> link reset induces a host link reset.  The current working theory is
> that yes, this must be the case.  If there is ever a case where a
> driver within the guest could trigger a link reset for the purposes
> of error recovery when the host has not, I think this must be the
> case.  Therefore, at least some guest induced link resets must become
> host link resets.  Currently we assume all guest induced link resets
> become host link resets.  Minimally to avoid that, QEMU would need to
> know (not assume) whether the host performed a link reset.  Even with
> that, QEMU would need to be able to correlate that a link reset from
> the guest is a duplicate of a link reset that was already performed by
> the host.  That implies that QEMU needs to deduce the intention of
> the guest.  That seems like a complicated task for a patch series that
> is already complicated enough, especially for a feature of questionable
> value given the configuration restrictions (imo).
> 
> I would much rather focus on getting it right and making it as simple
> as we can, even if that means links get reset one too many times on
> error.
> 

Thanks very much for your detailed explanation, it does helps me to
understand your concern, understand why a second link reset is necessary.

I still want to share my thoughts with you(not argue): now we know host
aer driver will do link reset for vfio-pci first, so I can say, even if
fatal error is link related, after host link reset, link can work now.
Then in qemu, we are not necessary to translate guest link reset to host
link reset, just use vfio_pci_reset() as it is to do device
reset(probably is FLR). Which also means we don't need following
patch(make code easier):

@@ -3120,6 +3122,18 @@ static void vfio_pci_reset(DeviceState *dev)

      trace_vfio_pci_reset(vdev->vbasedev.name);

+     if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
+         PCIDevice *br = pci_bridge_get_device(pdev->bus);
+
+         if ((pci_get_word(br->config + PCI_BRIDGE_CONTROL) &
+              PCI_BRIDGE_CTL_BUS_RESET)) {
+             if (pci_get_function_0(pdev) == pdev) {
+                 vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
+             }
+             return;
+         }
+     }
+
      vfio_pci_pre_reset(vdev);


I think this also implies: we have a virtual link in qemu, but a virtual
link will never be broken like a physical link.(In particular we already
know host aer driver surely will do link reset to recover physical
link). So, guest's link reset don't need to care whether virtual link is
reset, just care virtual device.  And qemu "translates guest link reset
to host link reset" seems kind of taking link-reset responsibility over
from host:)

>>>> 2. skip link reset in aer driver of host kernel, for vfio-pci.
>>>>    Let user decide how to do serious recovery
>>>>
>>>>    add new field "user_driver" in struct pci_dev, used to skip link
>>>>    reset for vfio-pci; add new field "link_reset" in struct
>>>>    vfio_pci_device to indicate link has been reset or not during
>>>>    recovery
>>>>
>>>>    - set user_driver in vfio_pci_probe(), to skip link reset for
>>>>      vfio-pci in host.
>>>>    - (use a flag)block user access(config, mmio) during host recovery
>>>>      (not sure if this step is necessary)
>>>>    - In qemu, translate guest link reset to host link reset.
>>>>    - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
>>>>      is executed
>>>>    - In vfio-pci driver's resume, new a timer, check "link_reset" field
>>>>      periodically, if it is set in reasonable time, then clear it and
>>>>      delete timer, or else, vfio-pci driver will does the link reset!  
>>>
>>> What happens in the case of a multifunction device where each function
>>> is part of a separate IOMMU group and one function is hot-removed from
>>> the user? We can't do a link reset on that function since the other
>>> function is still in use.  We have no choice but release a device in an
>>> unknown state back to the host.  
>>
>> hot-remove from user, do you mean, for example, all functions assigned
>> to VM, then suddenly a person does something like following
>>
>> $ echo 0000:06:00.0 > /sys/bus/pci/drivers/vfio-pci/unbind
>>
>> $ echo 0000:06:00.0 > /sys/bus/pci/drivers/igb/bind
>>
>> to return device to host driver, or don't bind it to host driver, let it
>> in driver-less state???
> 
> Yes, the host kernel has no visiblity to how a user is making use of
> devices.  To support AER we require a similar topology between host and
> guest such that a guest link reset translates to a host reset.  That
> requirement is imposed by userspace, ie. QEMU.  The host kernel cannot
> presume that this is the case.  Therefore we could have a
> multi-function device where each function is assigned to the same or
> different users in any configuration.  If a fault occurs and we defer
> to the user to perform the link reset, we have absolutely no guarantee
> that it will ever occur.  If the functions are assigned to different
> users, then each user individually doesn't have the capability to
> perform a link reset.  If the devices happen to be assigned to a single
> user when the error occurs, we cannot assume the user has an AER
> compatible configuration, the devices could be exposed as separate
> single function devices, any one of which might be individually removed
> from the user and made use of by the host, such as your sysfs example
> above.  The host cannot perform a link reset in this case either
> as the sibling devices are still in use by the guest.  Thanks,
> 
> Alex
> 
> 

this explanation is valuable to me, so this is also why we can't do link
reset in vfio driver when one of the function is closed. And do link
reset in vfio driver until all functions are close is poor solution and
very complex(quarantine the device) as you said.

I am going to try solution 1, but I still have some consideration share
with you, this won't stop my trial, and don't have relationship with
above discussion, just FYI:

In non-virtuallization environment, from device's perspective, the steps
of a normal recovery consists of:
    error_detect
    mmio_enabled
    link_reset
    slot_reset
    resume

Now in our condition, the steps become:
    *link_reset* (host's, the following are guest's)
    error_detect
    mmio_enabled
    link_reset
    slot_reset
    resume

Especially, some device's specific driver in guest could do some
specific work in error_detect, take igb_io_error_detected() for example.
Like the words in pci-error-recovery.txt said:

it gives the driver a chance to cleanup, waiting for pending stuff
(timers, whatever, etc...) to complete;

But if link_reset is the first step, we lost all the status(register
value, etc) in the device. Of course I don't know if this will be a
problem (might not), just curious if this has been your concern:)

-- 
Sincerely,
Cao jin

[toc] | [prev] | [next] | [standalone]

#1542798

On Thu, Dec 15, 2016 at 09:56:41PM +0800, Cao jin wrote:
> 
> 
> On 12/15/2016 06:16 AM, Alex Williamson wrote:
> > On Wed, 14 Dec 2016 18:24:23 +0800
> > Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> > 
> >> Sorry for late.
> >> after reading all your comments, I think I will try the solution 1.
> >>
> >> On 12/13/2016 03:12 AM, Alex Williamson wrote:
> >>> On Mon, 12 Dec 2016 21:49:01 +0800
> >>> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> >>>   
> >>>> Hi,
> >>>> I have 2 solutions(high level design) came to me, please see if they are
> >>>> acceptable, or which one is acceptable. Also have some questions.
> >>>>
> >>>> 1. block guest access during host recovery
> >>>>
> >>>>    add new field error_recovering in struct vfio_pci_device to
> >>>>    indicate host recovery status. aer driver in host will still do
> >>>>    reset link
> >>>>
> >>>>    - set error_recovering in vfio-pci driver's error_detected, used to
> >>>>      block all kinds of user access(config space, mmio)
> >>>>    - in order to solve concurrent issue of device resetting & user
> >>>>      access, check device state[*] in vfio-pci driver's resume, see if
> >>>>      device reset is done, if it is, then clear"error_recovering", or
> >>>>      else new a timer, check device state periodically until device
> >>>>      reset is done. (what if device reset don't end for a long time?)
> >>>>    - In qemu, translate guest link reset to host link reset.
> >>>>      A question here: we already have link reset in host, is a second
> >>>>      link reset necessary? why?
> >>>>  
> >>>>    [*] how to check device state: reading certain config space
> >>>>        register, check return value is valid or not(All F's)  
> >>>
> >>> Isn't this exactly the path we were on previously?  
> >>
> >> Yes, it is basically the previous path, plus the optimization.
> >>
> >>> There might be an
> >>> optimization that we could skip back-to-back resets, but how can you
> >>> necessarily infer that the resets are for the same thing? If the user
> >>> accesses the device between resets, can you still guarantee the guest
> >>> directed reset is unnecessary?  If time passes between resets, do you
> >>> know they're for the same event?  How much time can pass between the
> >>> host and guest reset to know they're for the same event?  In the
> >>> process of error handling, which is more important, speed or
> >>> correctness?
> >>>    
> >>
> >> I think vfio driver itself won't know what each reset comes for, and I
> >> don't quite understand why should vfio care this question, is this a new
> >> question in the design?
> > 
> > You're suggesting an optimization to eliminate one of the resets,
> > and as we've discussed, I don't see removing the host induced reset
> > as a viable option.  That means you want to eliminate the guest
> > directed reset.  There are potentially three levels to do that, the
> > vfio-pci driver in the host kernel, somewhere in QEMU, or eliminate it
> > within the guest.  My comments were directed to the first option, the
> > host kernel level cannot correlate user directed resets as duplicates
> > of host directed resets.  
> >  
> 
> Ah, maybe it is mistake, I don't really want to eliminate guest directed
> reset very much, I was just not sure why it is very necessary.
> 
> The optimization I said just is fully separating host recovery from
> guest recovery(timer, check device periodically) in time, because there
> is concurrent device resetting & user access.
> 
> >> But I think it make sense that the user access during 2 resets maybe a
> >> trouble for guest recovery, misbehaved user could be out of our
> >> imagination.  Correctness is more important.
> >>
> >> If I understand you right, let me make a summary: host recovery just
> >> does link reset, which is incomplete, so we'd better do a complete guest
> >> recovery for correctness.
> > 
> > We don't know whether the host link reset is incomplete, but we can't do
> > a link reset transparently to the device, the device is no longer in the
> > same state after the reset.  The device specific driver, which exists
> > in userspace needs to be involved in device recovery.  Therefore
> > regardless of how QEMU handles the error, the driver within the guest
> > needs to be notified and perform recovery.  Since the device is PCI and
> > we're on x86 and nobody wants to introduce paravirtual error recovery,
> > we must use AER.  Part of AER recovery includes the possibility of
> > performing a link reset.  So it seems this eliminates avoiding the link
> > reset within the guest.
> > 
> > That leaves QEMU.  Here we need to decide whether a guest triggered
> > link reset induces a host link reset.  The current working theory is
> > that yes, this must be the case.  If there is ever a case where a
> > driver within the guest could trigger a link reset for the purposes
> > of error recovery when the host has not, I think this must be the
> > case.  Therefore, at least some guest induced link resets must become
> > host link resets.  Currently we assume all guest induced link resets
> > become host link resets.  Minimally to avoid that, QEMU would need to
> > know (not assume) whether the host performed a link reset.  Even with
> > that, QEMU would need to be able to correlate that a link reset from
> > the guest is a duplicate of a link reset that was already performed by
> > the host.  That implies that QEMU needs to deduce the intention of
> > the guest.  That seems like a complicated task for a patch series that
> > is already complicated enough, especially for a feature of questionable
> > value given the configuration restrictions (imo).
> > 
> > I would much rather focus on getting it right and making it as simple
> > as we can, even if that means links get reset one too many times on
> > error.
> > 
> 
> Thanks very much for your detailed explanation, it does helps me to
> understand your concern, understand why a second link reset is necessary.
> 
> I still want to share my thoughts with you(not argue): now we know host
> aer driver will do link reset for vfio-pci first, so I can say, even if
> fatal error is link related, after host link reset, link can work now.
> Then in qemu, we are not necessary to translate guest link reset to host
> link reset, just use vfio_pci_reset() as it is to do device
> reset(probably is FLR). Which also means we don't need following
> patch(make code easier):
> 
> @@ -3120,6 +3122,18 @@ static void vfio_pci_reset(DeviceState *dev)
> 
>       trace_vfio_pci_reset(vdev->vbasedev.name);
> 
> +     if (vdev->features & VFIO_FEATURE_ENABLE_AER) {
> +         PCIDevice *br = pci_bridge_get_device(pdev->bus);
> +
> +         if ((pci_get_word(br->config + PCI_BRIDGE_CONTROL) &
> +              PCI_BRIDGE_CTL_BUS_RESET)) {
> +             if (pci_get_function_0(pdev) == pdev) {
> +                 vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
> +             }
> +             return;
> +         }
> +     }
> +
>       vfio_pci_pre_reset(vdev);
> 
> 
> I think this also implies: we have a virtual link in qemu, but a virtual
> link will never be broken like a physical link.(In particular we already
> know host aer driver surely will do link reset to recover physical
> link). So, guest's link reset don't need to care whether virtual link is
> reset, just care virtual device.  And qemu "translates guest link reset
> to host link reset" seems kind of taking link-reset responsibility over
> from host:)
> 
> >>>> 2. skip link reset in aer driver of host kernel, for vfio-pci.
> >>>>    Let user decide how to do serious recovery
> >>>>
> >>>>    add new field "user_driver" in struct pci_dev, used to skip link
> >>>>    reset for vfio-pci; add new field "link_reset" in struct
> >>>>    vfio_pci_device to indicate link has been reset or not during
> >>>>    recovery
> >>>>
> >>>>    - set user_driver in vfio_pci_probe(), to skip link reset for
> >>>>      vfio-pci in host.
> >>>>    - (use a flag)block user access(config, mmio) during host recovery
> >>>>      (not sure if this step is necessary)
> >>>>    - In qemu, translate guest link reset to host link reset.
> >>>>    - In vfio-pci driver, set link_reset after VFIO_DEVICE_PCI_HOT_RESET
> >>>>      is executed
> >>>>    - In vfio-pci driver's resume, new a timer, check "link_reset" field
> >>>>      periodically, if it is set in reasonable time, then clear it and
> >>>>      delete timer, or else, vfio-pci driver will does the link reset!  
> >>>
> >>> What happens in the case of a multifunction device where each function
> >>> is part of a separate IOMMU group and one function is hot-removed from
> >>> the user? We can't do a link reset on that function since the other
> >>> function is still in use.  We have no choice but release a device in an
> >>> unknown state back to the host.  
> >>
> >> hot-remove from user, do you mean, for example, all functions assigned
> >> to VM, then suddenly a person does something like following
> >>
> >> $ echo 0000:06:00.0 > /sys/bus/pci/drivers/vfio-pci/unbind
> >>
> >> $ echo 0000:06:00.0 > /sys/bus/pci/drivers/igb/bind
> >>
> >> to return device to host driver, or don't bind it to host driver, let it
> >> in driver-less state???
> > 
> > Yes, the host kernel has no visiblity to how a user is making use of
> > devices.  To support AER we require a similar topology between host and
> > guest such that a guest link reset translates to a host reset.  That
> > requirement is imposed by userspace, ie. QEMU.  The host kernel cannot
> > presume that this is the case.  Therefore we could have a
> > multi-function device where each function is assigned to the same or
> > different users in any configuration.  If a fault occurs and we defer
> > to the user to perform the link reset, we have absolutely no guarantee
> > that it will ever occur.  If the functions are assigned to different
> > users, then each user individually doesn't have the capability to
> > perform a link reset.  If the devices happen to be assigned to a single
> > user when the error occurs, we cannot assume the user has an AER
> > compatible configuration, the devices could be exposed as separate
> > single function devices, any one of which might be individually removed
> > from the user and made use of by the host, such as your sysfs example
> > above.  The host cannot perform a link reset in this case either
> > as the sibling devices are still in use by the guest.  Thanks,
> > 
> > Alex
> > 
> > 
> 
> this explanation is valuable to me, so this is also why we can't do link
> reset in vfio driver when one of the function is closed. And do link
> reset in vfio driver until all functions are close is poor solution and
> very complex(quarantine the device) as you said.
> 
> I am going to try solution 1, but I still have some consideration share
> with you, this won't stop my trial, and don't have relationship with
> above discussion, just FYI:
> 
> In non-virtuallization environment, from device's perspective, the steps
> of a normal recovery consists of:
>     error_detect
>     mmio_enabled
>     link_reset
>     slot_reset
>     resume
> 
> Now in our condition, the steps become:
>     *link_reset* (host's, the following are guest's)
>     error_detect
>     mmio_enabled
>     link_reset
>     slot_reset
>     resume
> 
> Especially, some device's specific driver in guest could do some
> specific work in error_detect, take igb_io_error_detected() for example.
> Like the words in pci-error-recovery.txt said:
> 
> it gives the driver a chance to cleanup, waiting for pending stuff
> (timers, whatever, etc...) to complete;
> 
> But if link_reset is the first step, we lost all the status(register
> value, etc) in the device. Of course I don't know if this will be a
> problem (might not), just curious if this has been your concern:)

You'll find I did mention it :)

But consider Documentation/PCI/pcieaer-howto.txt

	3.2.2.2 Non-correctable (non-fatal and fatal) errors

	If an error message indicates a non-fatal error, performing link reset
	at upstream is not required. The AER driver calls error_detected(dev,
	pci_channel_io_normal) to all drivers associated within a hierarchy in
	question. for example,
	EndPoint<==>DownstreamPort B<==>UpstreamPort A<==>RootPort.
	If Upstream port A captures an AER error, the hierarchy consists of
	Downstream port B and EndPoint.

	A driver may return PCI_ERS_RESULT_CAN_RECOVER,
	PCI_ERS_RESULT_DISCONNECT, or PCI_ERS_RESULT_NEED_RESET, depending on
	whether it can recover or the AER driver calls mmio_enabled as next.

	If an error message indicates a fatal error, kernel will broadcast
	error_detected(dev, pci_channel_io_frozen) to all drivers within
	a hierarchy in question. Then, performing link reset at upstream is
	necessary.

I think that if you just forward errors to guests they will get confused.
I see three possible approaches.


1. Always pretend to guest that there was a fatal error,
  then basically:

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index dce511f..4022f9b 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1299,7 +1299,7 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 
 	vfio_device_put(device);
 
-	return PCI_ERS_RESULT_CAN_RECOVER;
+	return PCI_ERS_RESULT_DISCONNECT;
 }
 
 static const struct pci_error_handlers vfio_err_handlers = {


probably conditional on userspace invoking some ioctl
to avoid breaking existing users.

2. send non fatal error to guest.
Add another eventfd to distinguish non fatal and fatal errors.

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index dce511f..e22f449 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1292,14 +1292,17 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 
 	mutex_lock(&vdev->igate);
 
-	if (vdev->err_trigger)
+	if (state == pci_channel_io_normal && vdev->recover_trigger)
+		eventfd_signal(vdev->recover_trigger, 1);
+	else if (vdev->err_trigger)
 		eventfd_signal(vdev->err_trigger, 1);
 
 	mutex_unlock(&vdev->igate);
 
 	vfio_device_put(device);
 
 	return PCI_ERS_RESULT_CAN_RECOVER;
 }
 
 static const struct pci_error_handlers vfio_err_handlers = {

Forward non fatal ones to guest, stop vm on fatal ones.



3. forward both non fatal and fatal error to guest
This includes 1 and 2 above, and

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index dce511f..4022f9b 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1299,7 +1299,8 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 
 	vfio_device_put(device);
 
-	return PCI_ERS_RESULT_CAN_RECOVER;
+	return state == pci_channel_io_normal : PCI_ERS_RESULT_CAN_RECOVER :
+		PCI_ERS_RESULT_DISCONNECT;
 }
 
 static const struct pci_error_handlers vfio_err_handlers = {
	
Maybe make this conditional on recover_trigger to keep
compatibility.


You seem to be starting from 1. But how about starting small, and doing
2 as a first step? Fatal errors will still stop vm.
This will help you merge a bunch of error reporting infrastructure
without worrying about recovery so much.

Making some progress finally will be good.


Alex, what do you think?


-- 
MST

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web