Re: [PATCH v6] vfio error recovery: kernel support
On Thu, 6 Apr 2017 16:53:44 +0800 Cao jin wrote: > On 04/06/2017 06:36 AM, Michael S. Tsirkin wrote: > > On Wed, Apr 05, 2017 at 04:19:10PM -0600, Alex Williamson wrote: > >> On Thu, 6 Apr 2017 00:50:22 +0300 > >> "Michael S. Tsirkin" wrote: > >> > >>> On Wed, Apr 05, 2017 at 01:38:22PM -0600, Alex Williamson wrote: > The previous intention of trying to handle all sorts of AER faults > clearly had more value, though even there the implementation and > configuration requirements restricted the practicality. For instance > is AER support actually useful to a customer if it requires all ports > of a multifunction device assigned to the VM? This seems more like a > feature targeting whole system partitioning rather than general VM > device assignment use cases. Maybe that's ok, but it should be a clear > design decision. > >>> > >>> Alex, what kind of testing do you expect to be necessary? > >>> Would you say testing on real hardware and making it trigger > >>> AER errors is a requirement? > >> > >> Testing various fatal, non-fatal, and corrected errors with aer-inject, > >> especially in multfunction configurations (where more than one port > >> is actually usable) would certainly be required. If we have cases where > >> the driver for a companion function can escalate a non-fatal error to a > >> bus reset, that should be tested, even if it requires temporary hacks to > >> the host driver for the companion function to trigger that case. AER > >> handling is not something that the typical user is going to experience, > >> so it should to be thoroughly tested to make sure it works when needed > >> or there's little point to doing it at all. Thanks, > >> > >> Alex > > > > Some things can be tested within a VM. What would you > > say would be sufficient on a VM and what has to be > > tested on bare metal? > > > > Does the "bare metal" here mean something like XenServer? No, bare metal means the non-virtualized host OS. I think Michael was trying to facilitate testing by proposing to do it in a VM such that we can create strange and interesting topologies that aren't bound by a system in a remote lab having only one NIC port connected. Thanks, Alex
Re: [PATCH v6] vfio error recovery: kernel support
On Thu, 6 Apr 2017 16:49:35 +0800 Cao jin wrote: > On 04/06/2017 05:56 AM, Michael S. Tsirkin wrote: > > On Wed, Apr 05, 2017 at 04:54:33PM +0800, Cao jin wrote: > >> Apparently, I don't have experience to induce non-fatal error, device > >> error is more of a chance related with the environment(temperature, > >> humidity, etc) as I understand. > > > > I'm not sure how to interpret this statement. I think what Alex is > > saying is simply that patches should include some justification. They > > make changes but what are they improving? > > For example: > > > > I tested device ABC in conditions DEF. Without a patch VM > > stops. With the patches applied VM recovers and proceeds to > > use the device normally. > > > > is one reasonable justification imho. > > > > Got it. But unfortunately, until now, I haven't seen a VM stop caused by > a real device non-fatal error during device assignment(Only saw real > fatal errors after start VM). > On one side, AER error could occur theoretically; on the other side, > seldom people have seen a VM stop caused by AER. Now I am asked that do > I have a real evidence or scenario to prove that this patchset is really > useful? I don't, and we all know it is hard to trigger a real hardware > error, so, seems I am pushed into the corner. I guess these questions > also apply for AER driver's author, if the scenario is easy to > reproduce, there is no need to write aer_inject to fake errors. Yes, non-fatal errors are rare, so rare in fact that I wonder if anything actually implements them. Could we take a survey of Linux drivers with AER support and see which ones actually do anything useful on a non-fatal error? I know fatal errors exist, I've seen them. The situation I want to avoid is adding non-fatal AER support that never gets used and just gets in the way of the next attempt to support fatal error recovery. We're extending the user ABI here, which means that for any feature we add, we need to consider how we'll support it long term and how we'll deprecate it or further extend it when the next level of support comes. I'm reluctant to add code that only partially solves a problem and only addresses the side of the problem that we're not even sure occurs. Thanks, Alex
Re: [PATCH v6] vfio error recovery: kernel support
On 04/06/2017 03:38 AM, Alex Williamson wrote: > On Wed, 5 Apr 2017 16:54:33 +0800 > Cao jin wrote: > >> Sorry for late. Distracted by other problem for a while. >> >> On 03/31/2017 02:16 AM, Alex Williamson wrote: >>> On Thu, 30 Mar 2017 21:00:35 +0300 >> > > I also asked in my previous comments to provide examples of errors > that > might trigger correctable errors to the user, this comment seems to > have been missed. In my experience, AERs generated during device > assignment are generally hardware faults or induced by bad guest > drivers. These are cases where a single fatal error is an appropriate > and sufficient response. We've scaled back this support to the point > where we're only improving the situation of correctable errors and I'm > not convinced this is worthwhile and we're not simply checking a box > on > an ill-conceived marketing requirements document. Sorry. I noticed that question: "what actual errors do we expect userspace to see as non-fatal errors?", but I am confused about it. Correctable, non-fatal, fatal errors are clearly defined in PCIe spec, and Uncorrectable Error Severity Register will tell which is fatal, and which is non-fatal, this register is configurable, they are device specific as I guess. AER core driver distinguish them by pci_channel_io_normal/pci_channel_io_frozen, So I don't understand your question. Or Or, Do you mean we could list the default non-fatal error of Uncorrectable Error Severity Register which is provided by PCIe spec? >>> >>> I'm trying to ask why is this patch series useful. It's clearly >>> possible for us to signal non-fatal errors for a device to a guest, but >>> why is it necessarily a good idea to do so? What additional RAS >>> feature is gained by this? Can we give a single example of a real >>> world scenario where a guest has been shutdown due to a non-fatal error >>> that the guest driver would have handled? >> >> We've been discussing AER for months if not years. >> Isn't it a bit too late to ask whether AER recovery >> by guests it useful at all? > > > Years, but I think that is more indicative of the persistence of the > developers rather than growing acceptance on my part. For the majority > of that we were headed down the path of full AER support with the guest > able to invoke bus resets. It was a complicated solution, but it was > more clear that it had some value. Of course that's been derailed > due to various issues and we're now on this partial implementation that > only covers non-fatal errors that we assume the guest can recover from > without providing it mechanisms to do bus resets. Is there actual > value to this or are we just trying to fill an AER checkbox on > someone's marketing sheet? I don't think it's too much to ask for a > commit log to include evidence or discussion about how a feature is > actually a benefit to implement. Seems rather self evident but ok. So something like With this patch, guest is able to recover from non-fatal correctable errors - as opposed to stopping the guest with no ability to recover which was the only option previously. Would this address your question? >>> >>> >>> No, that's just restating the theoretical usefulness of this. Have you >>> ever seen a non-fatal error? Does it ever trigger? If we can't >>> provide a real world case of this being useful, can we at least discuss >>> the types of things that might trigger a non-fatal error for which the >>> guest could recover? In patch 3/3 Cao Jin claimed we have a 50% chance >>> of reducing VM stop conditions, but I suspect this is just a misuse of >>> statistics, just because there are two choices, fatal vs non-fatal, >>> does not make them equally likely. Do we have any idea of the >>> incidence rate of non-fatal errors? Is it non-zero? Thanks, >>> >> >> Apparently, I don't have experience to induce non-fatal error, device >> error is more of a chance related with the environment(temperature, >> humidity, etc) as I understand. >> >> After reading the discussion, can I construe that the old design with >> full AER support is preferred than this new one? The core issue of the >> old design is that the second host link reset make the subsequent >> guest's register reading fail, and I think this can be solved by test >> the device's accessibility(read device' register, all F's means >> unaccessible. IIRC, EEH of Power also use this way to test device's >> accessiblity) and delay guest's reading if device is temporarily >> unaccessible. > > What is the actual AER handling requirement you're trying to solve?
Re: [PATCH v6] vfio error recovery: kernel support
On 04/06/2017 05:56 AM, Michael S. Tsirkin wrote: > On Wed, Apr 05, 2017 at 04:54:33PM +0800, Cao jin wrote: >> Apparently, I don't have experience to induce non-fatal error, device >> error is more of a chance related with the environment(temperature, >> humidity, etc) as I understand. > > I'm not sure how to interpret this statement. I think what Alex is > saying is simply that patches should include some justification. They > make changes but what are they improving? > For example: > > I tested device ABC in conditions DEF. Without a patch VM > stops. With the patches applied VM recovers and proceeds to > use the device normally. > > is one reasonable justification imho. > Got it. But unfortunately, until now, I haven't seen a VM stop caused by a real device non-fatal error during device assignment(Only saw real fatal errors after start VM). On one side, AER error could occur theoretically; on the other side, seldom people have seen a VM stop caused by AER. Now I am asked that do I have a real evidence or scenario to prove that this patchset is really useful? I don't, and we all know it is hard to trigger a real hardware error, so, seems I am pushed into the corner. I guess these questions also apply for AER driver's author, if the scenario is easy to reproduce, there is no need to write aer_inject to fake errors. -- Sincerely, Cao jin
Re: [PATCH v6] vfio error recovery: kernel support
On 04/06/2017 06:36 AM, Michael S. Tsirkin wrote: > On Wed, Apr 05, 2017 at 04:19:10PM -0600, Alex Williamson wrote: >> On Thu, 6 Apr 2017 00:50:22 +0300 >> "Michael S. Tsirkin" wrote: >> >>> On Wed, Apr 05, 2017 at 01:38:22PM -0600, Alex Williamson wrote: The previous intention of trying to handle all sorts of AER faults clearly had more value, though even there the implementation and configuration requirements restricted the practicality. For instance is AER support actually useful to a customer if it requires all ports of a multifunction device assigned to the VM? This seems more like a feature targeting whole system partitioning rather than general VM device assignment use cases. Maybe that's ok, but it should be a clear design decision. >>> >>> Alex, what kind of testing do you expect to be necessary? >>> Would you say testing on real hardware and making it trigger >>> AER errors is a requirement? >> >> Testing various fatal, non-fatal, and corrected errors with aer-inject, >> especially in multfunction configurations (where more than one port >> is actually usable) would certainly be required. If we have cases where >> the driver for a companion function can escalate a non-fatal error to a >> bus reset, that should be tested, even if it requires temporary hacks to >> the host driver for the companion function to trigger that case. AER >> handling is not something that the typical user is going to experience, >> so it should to be thoroughly tested to make sure it works when needed >> or there's little point to doing it at all. Thanks, >> >> Alex > > Some things can be tested within a VM. What would you > say would be sufficient on a VM and what has to be > tested on bare metal? > Does the "bare metal" here mean something like XenServer? -- Sincerely, Cao jin
Re: [PATCH v6] vfio error recovery: kernel support
On Thu, 6 Apr 2017 01:36:31 +0300 "Michael S. Tsirkin" wrote: > On Wed, Apr 05, 2017 at 04:19:10PM -0600, Alex Williamson wrote: > > On Thu, 6 Apr 2017 00:50:22 +0300 > > "Michael S. Tsirkin" wrote: > > > > > On Wed, Apr 05, 2017 at 01:38:22PM -0600, Alex Williamson wrote: > > > > The previous intention of trying to handle all sorts of AER faults > > > > clearly had more value, though even there the implementation and > > > > configuration requirements restricted the practicality. For instance > > > > is AER support actually useful to a customer if it requires all ports > > > > of a multifunction device assigned to the VM? This seems more like a > > > > feature targeting whole system partitioning rather than general VM > > > > device assignment use cases. Maybe that's ok, but it should be a clear > > > > design decision. > > > > > > Alex, what kind of testing do you expect to be necessary? > > > Would you say testing on real hardware and making it trigger > > > AER errors is a requirement? > > > > Testing various fatal, non-fatal, and corrected errors with aer-inject, > > especially in multfunction configurations (where more than one port > > is actually usable) would certainly be required. If we have cases where > > the driver for a companion function can escalate a non-fatal error to a > > bus reset, that should be tested, even if it requires temporary hacks to > > the host driver for the companion function to trigger that case. AER > > handling is not something that the typical user is going to experience, > > so it should to be thoroughly tested to make sure it works when needed > > or there's little point to doing it at all. Thanks, > > > > Alex > > Some things can be tested within a VM. What would you > say would be sufficient on a VM and what has to be > tested on bare metal? Testing on a VM could be interesting for development, but I'd expect bare metal for validation, no offense. Bus reset timing can be different, error propagation can be different, etc. Thanks, Alex
Re: [PATCH v6] vfio error recovery: kernel support
On Wed, Apr 05, 2017 at 04:19:10PM -0600, Alex Williamson wrote: > On Thu, 6 Apr 2017 00:50:22 +0300 > "Michael S. Tsirkin" wrote: > > > On Wed, Apr 05, 2017 at 01:38:22PM -0600, Alex Williamson wrote: > > > The previous intention of trying to handle all sorts of AER faults > > > clearly had more value, though even there the implementation and > > > configuration requirements restricted the practicality. For instance > > > is AER support actually useful to a customer if it requires all ports > > > of a multifunction device assigned to the VM? This seems more like a > > > feature targeting whole system partitioning rather than general VM > > > device assignment use cases. Maybe that's ok, but it should be a clear > > > design decision. > > > > Alex, what kind of testing do you expect to be necessary? > > Would you say testing on real hardware and making it trigger > > AER errors is a requirement? > > Testing various fatal, non-fatal, and corrected errors with aer-inject, > especially in multfunction configurations (where more than one port > is actually usable) would certainly be required. If we have cases where > the driver for a companion function can escalate a non-fatal error to a > bus reset, that should be tested, even if it requires temporary hacks to > the host driver for the companion function to trigger that case. AER > handling is not something that the typical user is going to experience, > so it should to be thoroughly tested to make sure it works when needed > or there's little point to doing it at all. Thanks, > > Alex Some things can be tested within a VM. What would you say would be sufficient on a VM and what has to be tested on bare metal? -- MST
Re: [PATCH v6] vfio error recovery: kernel support
On Thu, 6 Apr 2017 00:50:22 +0300 "Michael S. Tsirkin" wrote: > On Wed, Apr 05, 2017 at 01:38:22PM -0600, Alex Williamson wrote: > > The previous intention of trying to handle all sorts of AER faults > > clearly had more value, though even there the implementation and > > configuration requirements restricted the practicality. For instance > > is AER support actually useful to a customer if it requires all ports > > of a multifunction device assigned to the VM? This seems more like a > > feature targeting whole system partitioning rather than general VM > > device assignment use cases. Maybe that's ok, but it should be a clear > > design decision. > > Alex, what kind of testing do you expect to be necessary? > Would you say testing on real hardware and making it trigger > AER errors is a requirement? Testing various fatal, non-fatal, and corrected errors with aer-inject, especially in multfunction configurations (where more than one port is actually usable) would certainly be required. If we have cases where the driver for a companion function can escalate a non-fatal error to a bus reset, that should be tested, even if it requires temporary hacks to the host driver for the companion function to trigger that case. AER handling is not something that the typical user is going to experience, so it should to be thoroughly tested to make sure it works when needed or there's little point to doing it at all. Thanks, Alex
Re: [PATCH v6] vfio error recovery: kernel support
On Wed, Apr 05, 2017 at 04:54:33PM +0800, Cao jin wrote: > Apparently, I don't have experience to induce non-fatal error, device > error is more of a chance related with the environment(temperature, > humidity, etc) as I understand. I'm not sure how to interpret this statement. I think what Alex is saying is simply that patches should include some justification. They make changes but what are they improving? For example: I tested device ABC in conditions DEF. Without a patch VM stops. With the patches applied VM recovers and proceeds to use the device normally. is one reasonable justification imho. -- MST
Re: [PATCH v6] vfio error recovery: kernel support
On Wed, Apr 05, 2017 at 01:38:22PM -0600, Alex Williamson wrote: > The previous intention of trying to handle all sorts of AER faults > clearly had more value, though even there the implementation and > configuration requirements restricted the practicality. For instance > is AER support actually useful to a customer if it requires all ports > of a multifunction device assigned to the VM? This seems more like a > feature targeting whole system partitioning rather than general VM > device assignment use cases. Maybe that's ok, but it should be a clear > design decision. Alex, what kind of testing do you expect to be necessary? Would you say testing on real hardware and making it trigger AER errors is a requirement? -- MST
Re: [PATCH v6] vfio error recovery: kernel support
On Wed, 5 Apr 2017 16:54:33 +0800 Cao jin wrote: > Sorry for late. Distracted by other problem for a while. > > On 03/31/2017 02:16 AM, Alex Williamson wrote: > > On Thu, 30 Mar 2017 21:00:35 +0300 > > >> > >>> > >>> I also asked in my previous comments to provide examples of errors > >>> that > >>> might trigger correctable errors to the user, this comment seems to > >>> have been missed. In my experience, AERs generated during device > >>> assignment are generally hardware faults or induced by bad guest > >>> drivers. These are cases where a single fatal error is an appropriate > >>> and sufficient response. We've scaled back this support to the point > >>> where we're only improving the situation of correctable errors and I'm > >>> not convinced this is worthwhile and we're not simply checking a box > >>> on > >>> an ill-conceived marketing requirements document. > >> > >> Sorry. I noticed that question: "what actual errors do we expect > >> userspace to see as non-fatal errors?", but I am confused about it. > >> Correctable, non-fatal, fatal errors are clearly defined in PCIe spec, > >> and Uncorrectable Error Severity Register will tell which is fatal, and > >> which is non-fatal, this register is configurable, they are device > >> specific as I guess. AER core driver distinguish them by > >> pci_channel_io_normal/pci_channel_io_frozen, So I don't understand > >> your > >> question. Or > >> > >> Or, Do you mean we could list the default non-fatal error of > >> Uncorrectable Error Severity Register which is provided by PCIe spec? > >> > > > > I'm trying to ask why is this patch series useful. It's clearly > > possible for us to signal non-fatal errors for a device to a guest, but > > why is it necessarily a good idea to do so? What additional RAS > > feature is gained by this? Can we give a single example of a real > > world scenario where a guest has been shutdown due to a non-fatal error > > that the guest driver would have handled? > > We've been discussing AER for months if not years. > Isn't it a bit too late to ask whether AER recovery > by guests it useful at all? > >>> > >>> > >>> Years, but I think that is more indicative of the persistence of the > >>> developers rather than growing acceptance on my part. For the majority > >>> of that we were headed down the path of full AER support with the guest > >>> able to invoke bus resets. It was a complicated solution, but it was > >>> more clear that it had some value. Of course that's been derailed > >>> due to various issues and we're now on this partial implementation that > >>> only covers non-fatal errors that we assume the guest can recover from > >>> without providing it mechanisms to do bus resets. Is there actual > >>> value to this or are we just trying to fill an AER checkbox on > >>> someone's marketing sheet? I don't think it's too much to ask for a > >>> commit log to include evidence or discussion about how a feature is > >>> actually a benefit to implement. > >> > >> Seems rather self evident but ok. So something like > >> > >> With this patch, guest is able to recover from non-fatal correctable > >> errors - as opposed to stopping the guest with no ability to > >> recover which was the only option previously. > >> > >> Would this address your question? > > > > > > No, that's just restating the theoretical usefulness of this. Have you > > ever seen a non-fatal error? Does it ever trigger? If we can't > > provide a real world case of this being useful, can we at least discuss > > the types of things that might trigger a non-fatal error for which the > > guest could recover? In patch 3/3 Cao Jin claimed we have a 50% chance > > of reducing VM stop conditions, but I suspect this is just a misuse of > > statistics, just because there are two choices, fatal vs non-fatal, > > does not make them equally likely. Do we have any idea of the > > incidence rate of non-fatal errors? Is it non-zero? Thanks, > > > > Apparently, I don't have experience to induce non-fatal error, device > error is more of a chance related with the environment(temperature, > humidity, etc) as I understand. > > After reading the discussion, can I construe that the old design with > full AER support is preferred than this new one? The core issue of the > old design is that the second host link reset make the subsequent > guest's register reading fail, and I think this can be solved by test > the device's accessibility(read device' register, all F's means > unaccessible. IIRC, EEH of Power also use this way to test device's > accessiblity) and delay guest's reading if device is temporarily > unaccessible. What is the actual AER handling requirement you're trying to solve? Is it simply to check a box on a marketing spec sheet for anything rel
Re: [PATCH v6] vfio error recovery: kernel support
Sorry for late. Distracted by other problem for a while. On 03/31/2017 02:16 AM, Alex Williamson wrote: > On Thu, 30 Mar 2017 21:00:35 +0300 >> >>> >>> I also asked in my previous comments to provide examples of errors that >>> might trigger correctable errors to the user, this comment seems to >>> have been missed. In my experience, AERs generated during device >>> assignment are generally hardware faults or induced by bad guest >>> drivers. These are cases where a single fatal error is an appropriate >>> and sufficient response. We've scaled back this support to the point >>> where we're only improving the situation of correctable errors and I'm >>> not convinced this is worthwhile and we're not simply checking a box on >>> an ill-conceived marketing requirements document. >> >> Sorry. I noticed that question: "what actual errors do we expect >> userspace to see as non-fatal errors?", but I am confused about it. >> Correctable, non-fatal, fatal errors are clearly defined in PCIe spec, >> and Uncorrectable Error Severity Register will tell which is fatal, and >> which is non-fatal, this register is configurable, they are device >> specific as I guess. AER core driver distinguish them by >> pci_channel_io_normal/pci_channel_io_frozen, So I don't understand your >> question. Or >> >> Or, Do you mean we could list the default non-fatal error of >> Uncorrectable Error Severity Register which is provided by PCIe spec? > > I'm trying to ask why is this patch series useful. It's clearly > possible for us to signal non-fatal errors for a device to a guest, but > why is it necessarily a good idea to do so? What additional RAS > feature is gained by this? Can we give a single example of a real > world scenario where a guest has been shutdown due to a non-fatal error > that the guest driver would have handled? We've been discussing AER for months if not years. Isn't it a bit too late to ask whether AER recovery by guests it useful at all? >>> >>> >>> Years, but I think that is more indicative of the persistence of the >>> developers rather than growing acceptance on my part. For the majority >>> of that we were headed down the path of full AER support with the guest >>> able to invoke bus resets. It was a complicated solution, but it was >>> more clear that it had some value. Of course that's been derailed >>> due to various issues and we're now on this partial implementation that >>> only covers non-fatal errors that we assume the guest can recover from >>> without providing it mechanisms to do bus resets. Is there actual >>> value to this or are we just trying to fill an AER checkbox on >>> someone's marketing sheet? I don't think it's too much to ask for a >>> commit log to include evidence or discussion about how a feature is >>> actually a benefit to implement. >> >> Seems rather self evident but ok. So something like >> >> With this patch, guest is able to recover from non-fatal correctable >> errors - as opposed to stopping the guest with no ability to >> recover which was the only option previously. >> >> Would this address your question? > > > No, that's just restating the theoretical usefulness of this. Have you > ever seen a non-fatal error? Does it ever trigger? If we can't > provide a real world case of this being useful, can we at least discuss > the types of things that might trigger a non-fatal error for which the > guest could recover? In patch 3/3 Cao Jin claimed we have a 50% chance > of reducing VM stop conditions, but I suspect this is just a misuse of > statistics, just because there are two choices, fatal vs non-fatal, > does not make them equally likely. Do we have any idea of the > incidence rate of non-fatal errors? Is it non-zero? Thanks, > Apparently, I don't have experience to induce non-fatal error, device error is more of a chance related with the environment(temperature, humidity, etc) as I understand. After reading the discussion, can I construe that the old design with full AER support is preferred than this new one? The core issue of the old design is that the second host link reset make the subsequent guest's register reading fail, and I think this can be solved by test the device's accessibility(read device' register, all F's means unaccessible. IIRC, EEH of Power also use this way to test device's accessiblity) and delay guest's reading if device is temporarily unaccessible. -- Sincerely, Cao jin
Re: [PATCH v6] vfio error recovery: kernel support
On Thu, 30 Mar 2017 21:00:35 +0300 "Michael S. Tsirkin" wrote: > On Tue, Mar 28, 2017 at 08:55:13PM -0600, Alex Williamson wrote: > > On Wed, 29 Mar 2017 03:01:48 +0300 > > "Michael S. Tsirkin" wrote: > > > > > On Tue, Mar 28, 2017 at 10:12:33AM -0600, Alex Williamson wrote: > > > > On Tue, 28 Mar 2017 21:47:00 +0800 > > > > Cao jin wrote: > > > > > > > > > On 03/25/2017 06:12 AM, Alex Williamson wrote: > > > > > > On Thu, 23 Mar 2017 17:07:31 +0800 > > > > > > Cao jin wrote: > > > > > > > > > > > > A more appropriate patch subject would be: > > > > > > > > > > > > vfio-pci: Report correctable errors and slot reset events to user > > > > > > > > > > > > > > > > Correctable? It is confusing to me. Correctable error has its clear > > > > > definition in PCIe spec, shouldn't it be "non-fatal"? > > > > > > > > My mistake, non-fatal. > > > > > > > > > >> From: "Michael S. Tsirkin" > > > > > > > > > > > > This hardly seems accurate anymore. You could say Suggested-by and > > > > > > let > > > > > > Michael add a sign-off, but it's changed since he sent it. > > > > > > > > > > > >> > > > > > >> 0. What happens now (PCIE AER only) > > > > > >>Fatal errors cause a link reset. Non fatal errors don't. > > > > > >>All errors stop the QEMU guest eventually, but not immediately, > > > > > >>because it's detected and reported asynchronously. > > > > > >>Interrupts are forwarded as usual. > > > > > >>Correctable errors are not reported to user at all. > > > > > >> > > > > > >>Note: > > > > > >>PPC EEH is different, but this approach won't affect EEH. EEH > > > > > >> treat > > > > > >>all errors as fatal ones in AER, so they will still be > > > > > >> signalled to user > > > > > >>via the legacy eventfd. Besides, all devices/functions in a PE > > > > > >> belongs > > > > > >>to the same IOMMU group, so the slot_reset handler in this > > > > > >> approach > > > > > >>won't affect EEH either. > > > > > >> > > > > > >> 1. Correctable errors > > > > > >>Hardware can correct these errors without software intervention, > > > > > >>clear the error status is enough, this is what already done now. > > > > > >>No need to recover it, nothing changed, leave it as it is. > > > > > >> > > > > > >> 2. Fatal errors > > > > > >>They will induce a link reset. This is troublesome when user is > > > > > >>a QEMU guest. This approach doesn't touch the existing > > > > > >> mechanism. > > > > > >> > > > > > >> 3. Non-fatal errors > > > > > >>Before this patch, they are signalled to user the same way as > > > > > >> fatal ones. > > > > > >>With this patch, a new eventfd is introduced only for non-fatal > > > > > >> error > > > > > >>notification. By splitting non-fatal ones out, it will benefit > > > > > >> AER > > > > > >>recovery of a QEMU guest user. > > > > > >> > > > > > >>To maintain backwards compatibility with userspace, non-fatal > > > > > >> errors > > > > > >>will continue to trigger via the existing error interrupt index > > > > > >> if a > > > > > >>non-fatal signaling mechanism has not been registered. > > > > > >> > > > > > >>Note: > > > > > >>In case of PCI Express errors, kernel might request a slot reset > > > > > >>affecting our device (from our point of view this is a passive > > > > > >> device > > > > > >>reset as opposed to an active one requested by vfio itself). > > > > > >>This might currently happen if a slot reset is requested by a > > > > > >> driver > > > > > >>(other than vfio) bound to another device function in the same > > > > > >> slot. > > > > > >>This will cause our device to lose its state so report this > > > > > >> event to > > > > > >>userspace. > > > > > > > > > > > > I tried to convey this in my last comments, I don't think this is an > > > > > > appropriate commit log. Lead with what is the problem you're > > > > > > trying to > > > > > > fix and why, what is the benefit to the user, and how is the change > > > > > > accomplished. If you want to provide a State of Error Handling in > > > > > > VFIO, append it after the main points of the commit log. > > > > > > > > > > ok. > > > > > > > > > > > > > > > > > I also asked in my previous comments to provide examples of errors > > > > > > that > > > > > > might trigger correctable errors to the user, this comment seems to > > > > > > have been missed. In my experience, AERs generated during device > > > > > > assignment are generally hardware faults or induced by bad guest > > > > > > drivers. These are cases where a single fatal error is an > > > > > > appropriate > > > > > > and sufficient response. We've scaled back this support to the > > > > > > point > > > > > > where we're only improving the situation of correctable errors and > > > > > > I'm > > > > > > not convinced this is worthwhile and we're not simply checking a > > > > >
Re: [PATCH v6] vfio error recovery: kernel support
On Tue, Mar 28, 2017 at 08:55:13PM -0600, Alex Williamson wrote: > On Wed, 29 Mar 2017 03:01:48 +0300 > "Michael S. Tsirkin" wrote: > > > On Tue, Mar 28, 2017 at 10:12:33AM -0600, Alex Williamson wrote: > > > On Tue, 28 Mar 2017 21:47:00 +0800 > > > Cao jin wrote: > > > > > > > On 03/25/2017 06:12 AM, Alex Williamson wrote: > > > > > On Thu, 23 Mar 2017 17:07:31 +0800 > > > > > Cao jin wrote: > > > > > > > > > > A more appropriate patch subject would be: > > > > > > > > > > vfio-pci: Report correctable errors and slot reset events to user > > > > > > > > > > > > > Correctable? It is confusing to me. Correctable error has its clear > > > > definition in PCIe spec, shouldn't it be "non-fatal"? > > > > > > My mistake, non-fatal. > > > > > > > >> From: "Michael S. Tsirkin" > > > > > > > > > > This hardly seems accurate anymore. You could say Suggested-by and > > > > > let > > > > > Michael add a sign-off, but it's changed since he sent it. > > > > > > > > > >> > > > > >> 0. What happens now (PCIE AER only) > > > > >>Fatal errors cause a link reset. Non fatal errors don't. > > > > >>All errors stop the QEMU guest eventually, but not immediately, > > > > >>because it's detected and reported asynchronously. > > > > >>Interrupts are forwarded as usual. > > > > >>Correctable errors are not reported to user at all. > > > > >> > > > > >>Note: > > > > >>PPC EEH is different, but this approach won't affect EEH. EEH > > > > >> treat > > > > >>all errors as fatal ones in AER, so they will still be signalled > > > > >> to user > > > > >>via the legacy eventfd. Besides, all devices/functions in a PE > > > > >> belongs > > > > >>to the same IOMMU group, so the slot_reset handler in this > > > > >> approach > > > > >>won't affect EEH either. > > > > >> > > > > >> 1. Correctable errors > > > > >>Hardware can correct these errors without software intervention, > > > > >>clear the error status is enough, this is what already done now. > > > > >>No need to recover it, nothing changed, leave it as it is. > > > > >> > > > > >> 2. Fatal errors > > > > >>They will induce a link reset. This is troublesome when user is > > > > >>a QEMU guest. This approach doesn't touch the existing mechanism. > > > > >> > > > > >> 3. Non-fatal errors > > > > >>Before this patch, they are signalled to user the same way as > > > > >> fatal ones. > > > > >>With this patch, a new eventfd is introduced only for non-fatal > > > > >> error > > > > >>notification. By splitting non-fatal ones out, it will benefit AER > > > > >>recovery of a QEMU guest user. > > > > >> > > > > >>To maintain backwards compatibility with userspace, non-fatal > > > > >> errors > > > > >>will continue to trigger via the existing error interrupt index > > > > >> if a > > > > >>non-fatal signaling mechanism has not been registered. > > > > >> > > > > >>Note: > > > > >>In case of PCI Express errors, kernel might request a slot reset > > > > >>affecting our device (from our point of view this is a passive > > > > >> device > > > > >>reset as opposed to an active one requested by vfio itself). > > > > >>This might currently happen if a slot reset is requested by a > > > > >> driver > > > > >>(other than vfio) bound to another device function in the same > > > > >> slot. > > > > >>This will cause our device to lose its state so report this event > > > > >> to > > > > >>userspace. > > > > > > > > > > I tried to convey this in my last comments, I don't think this is an > > > > > appropriate commit log. Lead with what is the problem you're trying > > > > > to > > > > > fix and why, what is the benefit to the user, and how is the change > > > > > accomplished. If you want to provide a State of Error Handling in > > > > > VFIO, append it after the main points of the commit log. > > > > > > > > ok. > > > > > > > > > > > > > > I also asked in my previous comments to provide examples of errors > > > > > that > > > > > might trigger correctable errors to the user, this comment seems to > > > > > have been missed. In my experience, AERs generated during device > > > > > assignment are generally hardware faults or induced by bad guest > > > > > drivers. These are cases where a single fatal error is an appropriate > > > > > and sufficient response. We've scaled back this support to the point > > > > > where we're only improving the situation of correctable errors and I'm > > > > > not convinced this is worthwhile and we're not simply checking a box > > > > > on > > > > > an ill-conceived marketing requirements document. > > > > > > > > Sorry. I noticed that question: "what actual errors do we expect > > > > userspace to see as non-fatal errors?", but I am confused about it. > > > > Correctable, non-fatal, fatal errors are clearly defined in PCIe spec, > > > > and Uncorrectable Error Severity Register w
Re: [PATCH v6] vfio error recovery: kernel support
On Wed, 29 Mar 2017 03:01:48 +0300 "Michael S. Tsirkin" wrote: > On Tue, Mar 28, 2017 at 10:12:33AM -0600, Alex Williamson wrote: > > On Tue, 28 Mar 2017 21:47:00 +0800 > > Cao jin wrote: > > > > > On 03/25/2017 06:12 AM, Alex Williamson wrote: > > > > On Thu, 23 Mar 2017 17:07:31 +0800 > > > > Cao jin wrote: > > > > > > > > A more appropriate patch subject would be: > > > > > > > > vfio-pci: Report correctable errors and slot reset events to user > > > > > > > > > > Correctable? It is confusing to me. Correctable error has its clear > > > definition in PCIe spec, shouldn't it be "non-fatal"? > > > > My mistake, non-fatal. > > > > > >> From: "Michael S. Tsirkin" > > > > > > > > This hardly seems accurate anymore. You could say Suggested-by and let > > > > Michael add a sign-off, but it's changed since he sent it. > > > > > > > >> > > > >> 0. What happens now (PCIE AER only) > > > >>Fatal errors cause a link reset. Non fatal errors don't. > > > >>All errors stop the QEMU guest eventually, but not immediately, > > > >>because it's detected and reported asynchronously. > > > >>Interrupts are forwarded as usual. > > > >>Correctable errors are not reported to user at all. > > > >> > > > >>Note: > > > >>PPC EEH is different, but this approach won't affect EEH. EEH treat > > > >>all errors as fatal ones in AER, so they will still be signalled to > > > >> user > > > >>via the legacy eventfd. Besides, all devices/functions in a PE > > > >> belongs > > > >>to the same IOMMU group, so the slot_reset handler in this approach > > > >>won't affect EEH either. > > > >> > > > >> 1. Correctable errors > > > >>Hardware can correct these errors without software intervention, > > > >>clear the error status is enough, this is what already done now. > > > >>No need to recover it, nothing changed, leave it as it is. > > > >> > > > >> 2. Fatal errors > > > >>They will induce a link reset. This is troublesome when user is > > > >>a QEMU guest. This approach doesn't touch the existing mechanism. > > > >> > > > >> 3. Non-fatal errors > > > >>Before this patch, they are signalled to user the same way as fatal > > > >> ones. > > > >>With this patch, a new eventfd is introduced only for non-fatal > > > >> error > > > >>notification. By splitting non-fatal ones out, it will benefit AER > > > >>recovery of a QEMU guest user. > > > >> > > > >>To maintain backwards compatibility with userspace, non-fatal errors > > > >>will continue to trigger via the existing error interrupt index if a > > > >>non-fatal signaling mechanism has not been registered. > > > >> > > > >>Note: > > > >>In case of PCI Express errors, kernel might request a slot reset > > > >>affecting our device (from our point of view this is a passive > > > >> device > > > >>reset as opposed to an active one requested by vfio itself). > > > >>This might currently happen if a slot reset is requested by a driver > > > >>(other than vfio) bound to another device function in the same slot. > > > >>This will cause our device to lose its state so report this event to > > > >>userspace. > > > > > > > > I tried to convey this in my last comments, I don't think this is an > > > > appropriate commit log. Lead with what is the problem you're trying to > > > > fix and why, what is the benefit to the user, and how is the change > > > > accomplished. If you want to provide a State of Error Handling in > > > > VFIO, append it after the main points of the commit log. > > > > > > ok. > > > > > > > > > > > I also asked in my previous comments to provide examples of errors that > > > > might trigger correctable errors to the user, this comment seems to > > > > have been missed. In my experience, AERs generated during device > > > > assignment are generally hardware faults or induced by bad guest > > > > drivers. These are cases where a single fatal error is an appropriate > > > > and sufficient response. We've scaled back this support to the point > > > > where we're only improving the situation of correctable errors and I'm > > > > not convinced this is worthwhile and we're not simply checking a box on > > > > an ill-conceived marketing requirements document. > > > > > > Sorry. I noticed that question: "what actual errors do we expect > > > userspace to see as non-fatal errors?", but I am confused about it. > > > Correctable, non-fatal, fatal errors are clearly defined in PCIe spec, > > > and Uncorrectable Error Severity Register will tell which is fatal, and > > > which is non-fatal, this register is configurable, they are device > > > specific as I guess. AER core driver distinguish them by > > > pci_channel_io_normal/pci_channel_io_frozen, So I don't understand your > > > question. Or > > > > > > Or, Do you mean we could list the default non-fatal error of > > > Uncorrectable Error Severity Register which
Re: [PATCH v6] vfio error recovery: kernel support
On Tue, Mar 28, 2017 at 10:12:33AM -0600, Alex Williamson wrote: > On Tue, 28 Mar 2017 21:47:00 +0800 > Cao jin wrote: > > > On 03/25/2017 06:12 AM, Alex Williamson wrote: > > > On Thu, 23 Mar 2017 17:07:31 +0800 > > > Cao jin wrote: > > > > > > A more appropriate patch subject would be: > > > > > > vfio-pci: Report correctable errors and slot reset events to user > > > > > > > Correctable? It is confusing to me. Correctable error has its clear > > definition in PCIe spec, shouldn't it be "non-fatal"? > > My mistake, non-fatal. > > > >> From: "Michael S. Tsirkin" > > > > > > This hardly seems accurate anymore. You could say Suggested-by and let > > > Michael add a sign-off, but it's changed since he sent it. > > > > > >> > > >> 0. What happens now (PCIE AER only) > > >>Fatal errors cause a link reset. Non fatal errors don't. > > >>All errors stop the QEMU guest eventually, but not immediately, > > >>because it's detected and reported asynchronously. > > >>Interrupts are forwarded as usual. > > >>Correctable errors are not reported to user at all. > > >> > > >>Note: > > >>PPC EEH is different, but this approach won't affect EEH. EEH treat > > >>all errors as fatal ones in AER, so they will still be signalled to > > >> user > > >>via the legacy eventfd. Besides, all devices/functions in a PE > > >> belongs > > >>to the same IOMMU group, so the slot_reset handler in this approach > > >>won't affect EEH either. > > >> > > >> 1. Correctable errors > > >>Hardware can correct these errors without software intervention, > > >>clear the error status is enough, this is what already done now. > > >>No need to recover it, nothing changed, leave it as it is. > > >> > > >> 2. Fatal errors > > >>They will induce a link reset. This is troublesome when user is > > >>a QEMU guest. This approach doesn't touch the existing mechanism. > > >> > > >> 3. Non-fatal errors > > >>Before this patch, they are signalled to user the same way as fatal > > >> ones. > > >>With this patch, a new eventfd is introduced only for non-fatal error > > >>notification. By splitting non-fatal ones out, it will benefit AER > > >>recovery of a QEMU guest user. > > >> > > >>To maintain backwards compatibility with userspace, non-fatal errors > > >>will continue to trigger via the existing error interrupt index if a > > >>non-fatal signaling mechanism has not been registered. > > >> > > >>Note: > > >>In case of PCI Express errors, kernel might request a slot reset > > >>affecting our device (from our point of view this is a passive device > > >>reset as opposed to an active one requested by vfio itself). > > >>This might currently happen if a slot reset is requested by a driver > > >>(other than vfio) bound to another device function in the same slot. > > >>This will cause our device to lose its state so report this event to > > >>userspace. > > > > > > I tried to convey this in my last comments, I don't think this is an > > > appropriate commit log. Lead with what is the problem you're trying to > > > fix and why, what is the benefit to the user, and how is the change > > > accomplished. If you want to provide a State of Error Handling in > > > VFIO, append it after the main points of the commit log. > > > > ok. > > > > > > > > I also asked in my previous comments to provide examples of errors that > > > might trigger correctable errors to the user, this comment seems to > > > have been missed. In my experience, AERs generated during device > > > assignment are generally hardware faults or induced by bad guest > > > drivers. These are cases where a single fatal error is an appropriate > > > and sufficient response. We've scaled back this support to the point > > > where we're only improving the situation of correctable errors and I'm > > > not convinced this is worthwhile and we're not simply checking a box on > > > an ill-conceived marketing requirements document. > > > > Sorry. I noticed that question: "what actual errors do we expect > > userspace to see as non-fatal errors?", but I am confused about it. > > Correctable, non-fatal, fatal errors are clearly defined in PCIe spec, > > and Uncorrectable Error Severity Register will tell which is fatal, and > > which is non-fatal, this register is configurable, they are device > > specific as I guess. AER core driver distinguish them by > > pci_channel_io_normal/pci_channel_io_frozen, So I don't understand your > > question. Or > > > > Or, Do you mean we could list the default non-fatal error of > > Uncorrectable Error Severity Register which is provided by PCIe spec? > > I'm trying to ask why is this patch series useful. It's clearly > possible for us to signal non-fatal errors for a device to a guest, but > why is it necessarily a good idea to do so? What additional RAS > feature is gained by this? Can we give a single example of a real
Re: [PATCH v6] vfio error recovery: kernel support
On Tue, 28 Mar 2017 21:47:00 +0800 Cao jin wrote: > On 03/25/2017 06:12 AM, Alex Williamson wrote: > > On Thu, 23 Mar 2017 17:07:31 +0800 > > Cao jin wrote: > > > > A more appropriate patch subject would be: > > > > vfio-pci: Report correctable errors and slot reset events to user > > > > Correctable? It is confusing to me. Correctable error has its clear > definition in PCIe spec, shouldn't it be "non-fatal"? My mistake, non-fatal. > >> From: "Michael S. Tsirkin" > > > > This hardly seems accurate anymore. You could say Suggested-by and let > > Michael add a sign-off, but it's changed since he sent it. > > > >> > >> 0. What happens now (PCIE AER only) > >>Fatal errors cause a link reset. Non fatal errors don't. > >>All errors stop the QEMU guest eventually, but not immediately, > >>because it's detected and reported asynchronously. > >>Interrupts are forwarded as usual. > >>Correctable errors are not reported to user at all. > >> > >>Note: > >>PPC EEH is different, but this approach won't affect EEH. EEH treat > >>all errors as fatal ones in AER, so they will still be signalled to user > >>via the legacy eventfd. Besides, all devices/functions in a PE belongs > >>to the same IOMMU group, so the slot_reset handler in this approach > >>won't affect EEH either. > >> > >> 1. Correctable errors > >>Hardware can correct these errors without software intervention, > >>clear the error status is enough, this is what already done now. > >>No need to recover it, nothing changed, leave it as it is. > >> > >> 2. Fatal errors > >>They will induce a link reset. This is troublesome when user is > >>a QEMU guest. This approach doesn't touch the existing mechanism. > >> > >> 3. Non-fatal errors > >>Before this patch, they are signalled to user the same way as fatal > >> ones. > >>With this patch, a new eventfd is introduced only for non-fatal error > >>notification. By splitting non-fatal ones out, it will benefit AER > >>recovery of a QEMU guest user. > >> > >>To maintain backwards compatibility with userspace, non-fatal errors > >>will continue to trigger via the existing error interrupt index if a > >>non-fatal signaling mechanism has not been registered. > >> > >>Note: > >>In case of PCI Express errors, kernel might request a slot reset > >>affecting our device (from our point of view this is a passive device > >>reset as opposed to an active one requested by vfio itself). > >>This might currently happen if a slot reset is requested by a driver > >>(other than vfio) bound to another device function in the same slot. > >>This will cause our device to lose its state so report this event to > >>userspace. > > > > I tried to convey this in my last comments, I don't think this is an > > appropriate commit log. Lead with what is the problem you're trying to > > fix and why, what is the benefit to the user, and how is the change > > accomplished. If you want to provide a State of Error Handling in > > VFIO, append it after the main points of the commit log. > > ok. > > > > > I also asked in my previous comments to provide examples of errors that > > might trigger correctable errors to the user, this comment seems to > > have been missed. In my experience, AERs generated during device > > assignment are generally hardware faults or induced by bad guest > > drivers. These are cases where a single fatal error is an appropriate > > and sufficient response. We've scaled back this support to the point > > where we're only improving the situation of correctable errors and I'm > > not convinced this is worthwhile and we're not simply checking a box on > > an ill-conceived marketing requirements document. > > Sorry. I noticed that question: "what actual errors do we expect > userspace to see as non-fatal errors?", but I am confused about it. > Correctable, non-fatal, fatal errors are clearly defined in PCIe spec, > and Uncorrectable Error Severity Register will tell which is fatal, and > which is non-fatal, this register is configurable, they are device > specific as I guess. AER core driver distinguish them by > pci_channel_io_normal/pci_channel_io_frozen, So I don't understand your > question. Or > > Or, Do you mean we could list the default non-fatal error of > Uncorrectable Error Severity Register which is provided by PCIe spec? I'm trying to ask why is this patch series useful. It's clearly possible for us to signal non-fatal errors for a device to a guest, but why is it necessarily a good idea to do so? What additional RAS feature is gained by this? Can we give a single example of a real world scenario where a guest has been shutdown due to a non-fatal error that the guest driver would have handled? > > I had also commented asking how the hypervisor is expected to know > > whether the guest supports AER. With the existing support of a single > > fatal error, the
Re: [PATCH v6] vfio error recovery: kernel support
On 03/25/2017 06:12 AM, Alex Williamson wrote: > On Thu, 23 Mar 2017 17:07:31 +0800 > Cao jin wrote: > > A more appropriate patch subject would be: > > vfio-pci: Report correctable errors and slot reset events to user > Correctable? It is confusing to me. Correctable error has its clear definition in PCIe spec, shouldn't it be "non-fatal"? >> From: "Michael S. Tsirkin" > > This hardly seems accurate anymore. You could say Suggested-by and let > Michael add a sign-off, but it's changed since he sent it. > >> >> 0. What happens now (PCIE AER only) >>Fatal errors cause a link reset. Non fatal errors don't. >>All errors stop the QEMU guest eventually, but not immediately, >>because it's detected and reported asynchronously. >>Interrupts are forwarded as usual. >>Correctable errors are not reported to user at all. >> >>Note: >>PPC EEH is different, but this approach won't affect EEH. EEH treat >>all errors as fatal ones in AER, so they will still be signalled to user >>via the legacy eventfd. Besides, all devices/functions in a PE belongs >>to the same IOMMU group, so the slot_reset handler in this approach >>won't affect EEH either. >> >> 1. Correctable errors >>Hardware can correct these errors without software intervention, >>clear the error status is enough, this is what already done now. >>No need to recover it, nothing changed, leave it as it is. >> >> 2. Fatal errors >>They will induce a link reset. This is troublesome when user is >>a QEMU guest. This approach doesn't touch the existing mechanism. >> >> 3. Non-fatal errors >>Before this patch, they are signalled to user the same way as fatal ones. >>With this patch, a new eventfd is introduced only for non-fatal error >>notification. By splitting non-fatal ones out, it will benefit AER >>recovery of a QEMU guest user. >> >>To maintain backwards compatibility with userspace, non-fatal errors >>will continue to trigger via the existing error interrupt index if a >>non-fatal signaling mechanism has not been registered. >> >>Note: >>In case of PCI Express errors, kernel might request a slot reset >>affecting our device (from our point of view this is a passive device >>reset as opposed to an active one requested by vfio itself). >>This might currently happen if a slot reset is requested by a driver >>(other than vfio) bound to another device function in the same slot. >>This will cause our device to lose its state so report this event to >>userspace. > > I tried to convey this in my last comments, I don't think this is an > appropriate commit log. Lead with what is the problem you're trying to > fix and why, what is the benefit to the user, and how is the change > accomplished. If you want to provide a State of Error Handling in > VFIO, append it after the main points of the commit log. ok. > > I also asked in my previous comments to provide examples of errors that > might trigger correctable errors to the user, this comment seems to > have been missed. In my experience, AERs generated during device > assignment are generally hardware faults or induced by bad guest > drivers. These are cases where a single fatal error is an appropriate > and sufficient response. We've scaled back this support to the point > where we're only improving the situation of correctable errors and I'm > not convinced this is worthwhile and we're not simply checking a box on > an ill-conceived marketing requirements document. Sorry. I noticed that question: "what actual errors do we expect userspace to see as non-fatal errors?", but I am confused about it. Correctable, non-fatal, fatal errors are clearly defined in PCIe spec, and Uncorrectable Error Severity Register will tell which is fatal, and which is non-fatal, this register is configurable, they are device specific as I guess. AER core driver distinguish them by pci_channel_io_normal/pci_channel_io_frozen, So I don't understand your question. Or Or, Do you mean we could list the default non-fatal error of Uncorrectable Error Severity Register which is provided by PCIe spec? > > I had also commented asking how the hypervisor is expected to know > whether the guest supports AER. With the existing support of a single > fatal error, the hypervisor halts the VM regardless of the error > severity or guest support. Now we have the opportunity that the > hypervisor can forward a correctable error to the guest... and hope the > right thing occurs? I never saw any response to this comment. > I noticed this question, you said: "That doesn't imply a problem with this approach, the user (hypervisor) would be at fault for any difference in handling in that case.". Maybe I understand you wrong. >From my limit understanding, QEMU doesn't has a way to know whether a guest has AER support, AER support need several kbuild configuration, I don't know how qemu is expected to know these. >> >>
Re: [PATCH v6] vfio error recovery: kernel support
On Thu, 23 Mar 2017 17:07:31 +0800 Cao jin wrote: A more appropriate patch subject would be: vfio-pci: Report correctable errors and slot reset events to user > From: "Michael S. Tsirkin" This hardly seems accurate anymore. You could say Suggested-by and let Michael add a sign-off, but it's changed since he sent it. > > 0. What happens now (PCIE AER only) >Fatal errors cause a link reset. Non fatal errors don't. >All errors stop the QEMU guest eventually, but not immediately, >because it's detected and reported asynchronously. >Interrupts are forwarded as usual. >Correctable errors are not reported to user at all. > >Note: >PPC EEH is different, but this approach won't affect EEH. EEH treat >all errors as fatal ones in AER, so they will still be signalled to user >via the legacy eventfd. Besides, all devices/functions in a PE belongs >to the same IOMMU group, so the slot_reset handler in this approach >won't affect EEH either. > > 1. Correctable errors >Hardware can correct these errors without software intervention, >clear the error status is enough, this is what already done now. >No need to recover it, nothing changed, leave it as it is. > > 2. Fatal errors >They will induce a link reset. This is troublesome when user is >a QEMU guest. This approach doesn't touch the existing mechanism. > > 3. Non-fatal errors >Before this patch, they are signalled to user the same way as fatal ones. >With this patch, a new eventfd is introduced only for non-fatal error >notification. By splitting non-fatal ones out, it will benefit AER >recovery of a QEMU guest user. > >To maintain backwards compatibility with userspace, non-fatal errors >will continue to trigger via the existing error interrupt index if a >non-fatal signaling mechanism has not been registered. > >Note: >In case of PCI Express errors, kernel might request a slot reset >affecting our device (from our point of view this is a passive device >reset as opposed to an active one requested by vfio itself). >This might currently happen if a slot reset is requested by a driver >(other than vfio) bound to another device function in the same slot. >This will cause our device to lose its state so report this event to >userspace. I tried to convey this in my last comments, I don't think this is an appropriate commit log. Lead with what is the problem you're trying to fix and why, what is the benefit to the user, and how is the change accomplished. If you want to provide a State of Error Handling in VFIO, append it after the main points of the commit log. I also asked in my previous comments to provide examples of errors that might trigger correctable errors to the user, this comment seems to have been missed. In my experience, AERs generated during device assignment are generally hardware faults or induced by bad guest drivers. These are cases where a single fatal error is an appropriate and sufficient response. We've scaled back this support to the point where we're only improving the situation of correctable errors and I'm not convinced this is worthwhile and we're not simply checking a box on an ill-conceived marketing requirements document. I had also commented asking how the hypervisor is expected to know whether the guest supports AER. With the existing support of a single fatal error, the hypervisor halts the VM regardless of the error severity or guest support. Now we have the opportunity that the hypervisor can forward a correctable error to the guest... and hope the right thing occurs? I never saw any response to this comment. > > Signed-off-by: Michael S. Tsirkin > Signed-off-by: Cao jin > --- > v6 changelog: > Address all the comments from MST. > > drivers/vfio/pci/vfio_pci.c | 49 > +++-- > drivers/vfio/pci/vfio_pci_intrs.c | 38 > drivers/vfio/pci/vfio_pci_private.h | 2 ++ > include/uapi/linux/vfio.h | 2 ++ > 4 files changed, 89 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index 324c52e..71f9a8a 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -441,7 +441,9 @@ static int vfio_pci_get_irq_count(struct vfio_pci_device > *vdev, int irq_type) > > return (flags & PCI_MSIX_FLAGS_QSIZE) + 1; > } > - } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX) { > + } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX || > +irq_type == VFIO_PCI_NON_FATAL_ERR_IRQ_INDEX || > +irq_type == VFIO_PCI_PASSIVE_RESET_IRQ_INDEX) { Should we add a typdef to alias VFIO_PCI_ERR_IRQ_INDEX to VFIO_PCI_FATAL_ERR_IRQ? > if (pci_is_pcie(vdev->pdev)) > return 1; > } else if (irq_type == VFIO_PCI_REQ_IRQ_INDEX) { > @@ -796,6 +798,8 @@ static lon