Re: [PATCH v3 0/2] VT-d flush issue

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

>On 12.12.2015 at 9:22pm, <quan.xu@intel.com> wrote:
> This patches are based on Kevin Tian's previous discussion 'Revisit 
>VT-d  asynchronous flush issue'.
> Fix current timeout concern and also allow limited ATS support in a light way:


> 2. Fix vt-d flush timeout issue.
> 
>     If IOTLB/Context/IETC flush is timeout, we should think all 
> devices under this IOMMU cannot function correctly.
>     So for each device under this IOMMU we'll mark it as unassignable 
> and kill the domain owning the device.
> 

Hi, 

Through research and investigation, when IEC/Iotlb/Context are flush error(VT-d is bug),
IMO it is unavoidable to panic. The following are some reasons:

1. The below is the general platform topology, illustrated by VT-d spec.
VT-d is a key component of the platform infrastructure in virtualization usage,
providing DMA/Intr remapping capabilities.
If such a key component VT-d is bug, it can't provide reliability for recording
and reporting of DMA/Intr error to VMM that may otherwise corrupt memory or
impact VM isolation.


       Processor  ... Processor
       ---------      ---------
                  ^
                  |

             North Bridge
            --------------      <---> DRAM
           DMA/Intr Remapping

                ^^^^
                |||| PCIe Devices
                vvvv



2. If VT-d is bug, does the hardware_domain continue to work with PCIe Devices / DRAM well with DMA remapping error?
   I think it is no. furthermore, i think VMM can NOT run a normal HVM domain without device-passthrough.

3. There are so many reasons for IEC/iotlb/Conetxt flush, .i.e. msi/ept... update.
   It distributed across the VMM source code, it is challenge to make sure callers actually honor errors
   and check all the way up the call trees. it looks like rewriting VMM source code.

4. Much more detail, some flush errors are very tricky. .i.e. how to deal with msi free with IEC flush error,
   restore or ignore it?


Welcome your comments and correct me if i am wrong. thanks.

-Quan



>     If Device-TLB flush is timeout, we'll mark the target ATS device 
> as unassignable and kill the domain owning
>     this device.
> 
>     If impacted domain is hardware domain, just throw out a warning. 
> It's an open here whether we want to kill
>     hardware domain (or directly panic hypervisor). Comments are welcomed.
> 
>     Device marked as unassignable will be disallowed to be further 
> assigned to any domain.

Re: [PATCH v3 0/2] VT-d flush issue

[Andrew Cooper]

On 20/12/15 13:57, Xu, Quan wrote:
>> On 12.12.2015 at 9:22pm, <quan.xu@intel.com> wrote:
>> This patches are based on Kevin Tian's previous discussion 'Revisit 
>> VT-d  asynchronous flush issue'.
>> Fix current timeout concern and also allow limited ATS support in a light way:
>
>> 2. Fix vt-d flush timeout issue.
>>
>>     If IOTLB/Context/IETC flush is timeout, we should think all 
>> devices under this IOMMU cannot function correctly.
>>     So for each device under this IOMMU we'll mark it as unassignable 
>> and kill the domain owning the device.
>>
> Hi, 
>
> Through research and investigation, when IEC/Iotlb/Context are flush error(VT-d is bug),
> IMO it is unavoidable to panic. The following are some reasons:
>
> 1. The below is the general platform topology, illustrated by VT-d spec.
> VT-d is a key component of the platform infrastructure in virtualization usage,
> providing DMA/Intr remapping capabilities.
> If such a key component VT-d is bug, it can't provide reliability for recording
> and reporting of DMA/Intr error to VMM that may otherwise corrupt memory or
> impact VM isolation.
>
>
>        Processor  ... Processor
>        ---------      ---------
>                   ^
>                   |
>
>              North Bridge
>             --------------      <---> DRAM
>            DMA/Intr Remapping
>
>                 ^^^^
>                 |||| PCIe Devices
>                 vvvv
>
>
>
> 2. If VT-d is bug, does the hardware_domain continue to work with PCIe Devices / DRAM well with DMA remapping error?
>    I think it is no. furthermore, i think VMM can NOT run a normal HVM domain without device-passthrough.
>
> 3. There are so many reasons for IEC/iotlb/Conetxt flush, .i.e. msi/ept... update.
>    It distributed across the VMM source code, it is challenge to make sure callers actually honor errors
>    and check all the way up the call trees. it looks like rewriting VMM source code.
>
> 4. Much more detail, some flush errors are very tricky. .i.e. how to deal with msi free with IEC flush error,
>    restore or ignore it?
>
>
> Welcome your comments and correct me if i am wrong. thanks.
>
> -Quan

I believe everywhere you say "is bug", you mean "is buggy".

I agree that, if the remapping engine itself is buggy, Xen can't be
certain about anything else functioning correctly.

However, there are many errors the remapping engine could generate which
are not because it itself is buggy.  These could be because of a
downstream device misbehaving, or because the remapping engine was
incorrectly programmed.  These cases should not be able to directly
cause a crash.

In the case of a buggy device, it doesn't matter if it, and all its
resources, are left in stuck state; it can be ignored and the rest of
the host can try to continue normally.

For point 3 specifically.  The code is indeed currently broken, and
needs fixing, one way or another.  It is sad that we are still suffering
from lots of very poor quality code submitted in the past.

Even in the case that we discover that a remapping engine itself is
buggy, it is likely not to be the only remapping engine in the system. 
If it can be safely disabled, the host has a good chance of being able
to continue sensibly.

~Andrew

Re: [PATCH v3 0/2] VT-d flush issue

[Jan Beulich]

>>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
> 2. If VT-d is bug, does the hardware_domain continue to work with PCIe 
> Devices / DRAM well with DMA remapping error?
>    I think it is no. furthermore, i think VMM can NOT run a normal HVM 
> domain without device-passthrough.

In addition to what Andrew said - VT-d is effectively not in use for
domains without PT device. Impacting all such domains by crashing
the hypervisor just because (in the extreme case) a single domain
with PT devices exhibited a flush issue is a no-go imo.

Jan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
> > 2. If VT-d is bug, does the hardware_domain continue to work with PCIe
> > Devices / DRAM well with DMA remapping error?
> >    I think it is no. furthermore, i think VMM can NOT run a normal HVM
> > domain without device-passthrough.
> 
> In addition to what Andrew said - VT-d is effectively not in use for domains
> without PT device.

IMO, When VT-d is enabled, but is not working correct. These PCI-e devices (Disks/NICs..) DMA/Interrupt behaviors are not predictable. 
Assumed that, VT-d is effectively not in use for domains without PT device, while at least the virtualization infrastructure is not trusted.
I think it is also not secure to run PV domains.

> Impacting all such domains by crashing the hypervisor just
> because (in the extreme case) a single domain with PT devices exhibited a flush
> issue is a no-go imo.
> 

IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single domain behavior, it is a Hypervisor and infrastructure issue.
ATS device's Device-TLB flush is a single domain issue.
Back to our original goal, my patch set is for ATS flush issue. right?

Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
> > 2. If VT-d is bug, does the hardware_domain continue to work with 
> > PCIe Devices / DRAM well with DMA remapping error?
> >    I think it is no. furthermore, i think VMM can NOT run a normal 
> > HVM domain without device-passthrough.
> 
> In addition to what Andrew said - VT-d is effectively not in use for 
> domains without PT device.

IMO, When VT-d is enabled, but is not working correct. These PCI-e devices (Disks/NICs..) DMA/Interrupt behaviors are not predictable. 
Assumed that, VT-d is effectively not in use for domains without PT device, while at least the virtualization infrastructure is not trusted.
I think it is also not secure to run PV domains.

> Impacting all such domains by crashing the hypervisor just because (in 
> the extreme case) a single domain with PT devices exhibited a flush 
> issue is a no-go imo.
> 

IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single domain behavior, it is a Hypervisor and infrastructure issue.
ATS device's Device-TLB flush is a single domain issue.
Back to our original goal, my patch set is for ATS flush issue. right?

Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Jan Beulich]

>>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
> On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
>> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
>> > 2. If VT-d is bug, does the hardware_domain continue to work with PCIe
>> > Devices / DRAM well with DMA remapping error?
>> >    I think it is no. furthermore, i think VMM can NOT run a normal HVM
>> > domain without device-passthrough.
>> 
>> In addition to what Andrew said - VT-d is effectively not in use for domains
>> without PT device.
> 
> IMO, When VT-d is enabled, but is not working correct. These PCI-e devices 
> (Disks/NICs..) DMA/Interrupt behaviors are not predictable. 
> Assumed that, VT-d is effectively not in use for domains without PT device, 
> while at least the virtualization infrastructure is not trusted.
> I think it is also not secure to run PV domains.
> 
>> Impacting all such domains by crashing the hypervisor just
>> because (in the extreme case) a single domain with PT devices exhibited a flush
>> issue is a no-go imo.
>> 
> 
> IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single domain behavior, 
> it is a Hypervisor and infrastructure issue.
> ATS device's Device-TLB flush is a single domain issue.
> Back to our original goal, my patch set is for ATS flush issue. right?

You mean you don't like this entailing clean up of other code? I'm
sorry, but I'm afraid you won't get away without - perhaps the
VT-d maintainers could help here, but in the end you have to face
that it was mainly Intel people who introduced the code which now
needs fixing up, so I consider it not exactly unfair for you (as a
company) to do this work.

Jan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

> On 21.12.2015 at 8:50pm, <JBeulich@suse.com> wrote:
> >>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
> > On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
> >> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
> >> > 2. If VT-d is bug, does the hardware_domain continue to work with
> >> > PCIe Devices / DRAM well with DMA remapping error?
> >> >    I think it is no. furthermore, i think VMM can NOT run a normal
> >> > HVM domain without device-passthrough.
> >>
> >> In addition to what Andrew said - VT-d is effectively not in use for
> >> domains without PT device.
> >
> > IMO, When VT-d is enabled, but is not working correct. These PCI-e
> > devices
> > (Disks/NICs..) DMA/Interrupt behaviors are not predictable.
> > Assumed that, VT-d is effectively not in use for domains without PT
> > device, while at least the virtualization infrastructure is not trusted.
> > I think it is also not secure to run PV domains.
> >
> >> Impacting all such domains by crashing the hypervisor just because
> >> (in the extreme case) a single domain with PT devices exhibited a
> >> flush issue is a no-go imo.
> >>
> >
> > IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single domain
> > behavior, it is a Hypervisor and infrastructure issue.
> > ATS device's Device-TLB flush is a single domain issue.
> > Back to our original goal, my patch set is for ATS flush issue. right?
> 
> You mean you don't like this entailing clean up of other code? 

 Jan, for ARM/AMD, I really have no knowledge to fix it. and I have no
 ARM/AMD hardware to verify it. if I need to fix these common part of INTEL/ARM/AMD, I think I need to make
 Xen compile correct and not to destroy the logic.

> I'm sorry, but I'm
> afraid you won't get away without - perhaps the VT-d maintainers could help
> here, but in the end you have to face that it was mainly Intel people who
> introduced the code which now needs fixing up, so I consider it not exactly
> unfair for you (as a
> company) to do this work.
> 

Furthermore, I found out that
     if IEC/Iotlb/Context flush error, then panic.
     Else if device-tlb flush error, we'll hide the target ATS device and kill the domain owning this ATS device. If impacted domain is hardware domain, just throw out a warning.

     Then, it is fine to _not_check all the way up the device-tlb flush call trees( maybe it is our next topic of discussion). Then it will not hurt the ARM/AMD platform.

               Quan 














> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v3 0/2] VT-d flush issue

[Jan Beulich]

>>> On 21.12.15 at 14:08, <quan.xu@intel.com> wrote:
>>  On 21.12.2015 at 8:50pm, <JBeulich@suse.com> wrote:
>> >>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
>> > On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
>> >> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
>> >> > 2. If VT-d is bug, does the hardware_domain continue to work with
>> >> > PCIe Devices / DRAM well with DMA remapping error?
>> >> >    I think it is no. furthermore, i think VMM can NOT run a normal
>> >> > HVM domain without device-passthrough.
>> >>
>> >> In addition to what Andrew said - VT-d is effectively not in use for
>> >> domains without PT device.
>> >
>> > IMO, When VT-d is enabled, but is not working correct. These PCI-e
>> > devices
>> > (Disks/NICs..) DMA/Interrupt behaviors are not predictable.
>> > Assumed that, VT-d is effectively not in use for domains without PT
>> > device, while at least the virtualization infrastructure is not trusted.
>> > I think it is also not secure to run PV domains.
>> >
>> >> Impacting all such domains by crashing the hypervisor just because
>> >> (in the extreme case) a single domain with PT devices exhibited a
>> >> flush issue is a no-go imo.
>> >>
>> >
>> > IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single domain
>> > behavior, it is a Hypervisor and infrastructure issue.
>> > ATS device's Device-TLB flush is a single domain issue.
>> > Back to our original goal, my patch set is for ATS flush issue. right?
>> 
>> You mean you don't like this entailing clean up of other code? 
> 
>  Jan, for ARM/AMD, I really have no knowledge to fix it. and I have no
>  ARM/AMD hardware to verify it. if I need to fix these common part of 
> INTEL/ARM/AMD, I think I need to make
>  Xen compile correct and not to destroy the logic.

You indeed aren't expected to fix AMD or ARM code, but it may be
necessary to adjust that code to make error propagation work.

>> I'm sorry, but I'm
>> afraid you won't get away without - perhaps the VT-d maintainers could help
>> here, but in the end you have to face that it was mainly Intel people who
>> introduced the code which now needs fixing up, so I consider it not exactly
>> unfair for you (as a
>> company) to do this work.
>> 
> 
> Furthermore, I found out that
>      if IEC/Iotlb/Context flush error, then panic.
>      Else if device-tlb flush error, we'll hide the target ATS device and 
> kill the domain owning this ATS device. If impacted domain is hardware 
> domain, just throw out a warning.
> 
>      Then, it is fine to _not_check all the way up the device-tlb flush call 
> trees( maybe it is our next topic of discussion).

I don't follow - this sounds more or less like the model you've been
following in past versions, yet it was that which prompted the
request to properly propagate errors.

Jan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

> On 21.12.2015 at 9:23pm, <JBeulich@suse.com> wrote:
> >>> On 21.12.15 at 14:08, <quan.xu@intel.com> wrote:
> >>  On 21.12.2015 at 8:50pm, <JBeulich@suse.com> wrote:
> >> >>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
> >> > On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
> >> >> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
> >> >> > 2. If VT-d is bug, does the hardware_domain continue to work
> >> >> > with PCIe Devices / DRAM well with DMA remapping error?
> >> >> >    I think it is no. furthermore, i think VMM can NOT run a
> >> >> > normal HVM domain without device-passthrough.
> >> >>
> >> >> In addition to what Andrew said - VT-d is effectively not in use
> >> >> for domains without PT device.
> >> >
> >> > IMO, When VT-d is enabled, but is not working correct. These PCI-e
> >> > devices
> >> > (Disks/NICs..) DMA/Interrupt behaviors are not predictable.
> >> > Assumed that, VT-d is effectively not in use for domains without PT
> >> > device, while at least the virtualization infrastructure is not trusted.
> >> > I think it is also not secure to run PV domains.
> >> >
> >> >> Impacting all such domains by crashing the hypervisor just because
> >> >> (in the extreme case) a single domain with PT devices exhibited a
> >> >> flush issue is a no-go imo.
> >> >>
> >> >
> >> > IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single domain
> >> > behavior, it is a Hypervisor and infrastructure issue.
> >> > ATS device's Device-TLB flush is a single domain issue.
> >> > Back to our original goal, my patch set is for ATS flush issue. right?
> >>
> >> You mean you don't like this entailing clean up of other code?
> >
> >  Jan, for ARM/AMD, I really have no knowledge to fix it. and I have no
> > ARM/AMD hardware to verify it. if I need to fix these common part of
> > INTEL/ARM/AMD, I think I need to make  Xen compile correct and not to
> > destroy the logic.
> 
> You indeed aren't expected to fix AMD or ARM code, but it may be necessary to
> adjust that code to make error propagation work.
> 
> >> I'm sorry, but I'm
> >> afraid you won't get away without - perhaps the VT-d maintainers
> >> could help here, but in the end you have to face that it was mainly
> >> Intel people who introduced the code which now needs fixing up, so I
> >> consider it not exactly unfair for you (as a
> >> company) to do this work.
> >>
> >
> > Furthermore, I found out that
> >      if IEC/Iotlb/Context flush error, then panic.
> >      Else if device-tlb flush error, we'll hide the target ATS device
> > and kill the domain owning this ATS device. If impacted domain is
> > hardware domain, just throw out a warning.
> >
> >      Then, it is fine to _not_check all the way up the device-tlb
> > flush call trees( maybe it is our next topic of discussion).
> 
> I don't follow - this sounds more or less like the model you've been following in
> past versions, yet it was that which prompted the request to properly propagate
> errors.
> 
Jan,
Maybe we can discuss the big picture first on how to deal with iec/iotlb/context and Device-TLB flush error.
Then we can discuss it in detail. We can ignore some point of the way up the device-tlb flush call trees. Such as 

   iommu_hwdom_init()
   *|--hd->platform_ops->map_page(d, gfn, mfn, mapping);


And more, if we are on same page, I am glad to write patch for all of vt-d issue, including IOMMU_WAIT_OP issue .etc..

-Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

> On 21.12.2015 at 9:23pm, <JBeulich@suse.com> wrote:
> >>> On 21.12.15 at 14:08, <quan.xu@intel.com> wrote:
> >>  On 21.12.2015 at 8:50pm, <JBeulich@suse.com> wrote:
> >> >>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
> >> > On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
> >> >> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:

1. 
> >> > IMO, When VT-d is enabled, but is not working correct. These PCI-e
> >> > devices
> >> > (Disks/NICs..) DMA/Interrupt behaviors are not predictable.
> >> > Assumed that, VT-d is effectively not in use for domains without PT
> >> > device, while at least the virtualization infrastructure is not trusted.
> >> > I think it is also not secure to run PV domains.
> >> >

2. 
> >> > IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single domain
> >> > behavior, it is a Hypervisor and infrastructure issue.
> >> > ATS device's Device-TLB flush is a single domain issue.
> >> > Back to our original goal, my patch set is for ATS flush issue. right?
> >>

One quick question, 
Jan, do you agreed the above 2 descriptions?

Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Jan Beulich]

>>> On 21.12.15 at 15:08, <quan.xu@intel.com> wrote:
>>  On 21.12.2015 at 9:23pm, <JBeulich@suse.com> wrote:
>> >>> On 21.12.15 at 14:08, <quan.xu@intel.com> wrote:
>> >>  On 21.12.2015 at 8:50pm, <JBeulich@suse.com> wrote:
>> >> >>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
>> >> > On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
>> >> >> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
> 
> 1. 
>> >> > IMO, When VT-d is enabled, but is not working correct. These PCI-e
>> >> > devices
>> >> > (Disks/NICs..) DMA/Interrupt behaviors are not predictable.
>> >> > Assumed that, VT-d is effectively not in use for domains without PT
>> >> > device, while at least the virtualization infrastructure is not trusted.
> 
> 2. 
>> >> > IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single domain
>> >> > behavior, it is a Hypervisor and infrastructure issue.
>> >> > ATS device's Device-TLB flush is a single domain issue.
> 
> One quick question, 
> Jan, do you agreed the above 2 descriptions?

I agree, but (see also Andrew's earlier reply) don't take them as an
excuse to crash Xen upon flush failures. Please accept that the
general policy has to be to handle errors with as narrow an impact
as possible.

Jan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

> On 21.12.2015 at 10:16pm, <JBeulich@suse.com> wrote:
> >>> On 21.12.15 at 15:08, <quan.xu@intel.com> wrote:
> >>  On 21.12.2015 at 9:23pm, <JBeulich@suse.com> wrote:
> >> >>> On 21.12.15 at 14:08, <quan.xu@intel.com> wrote:
> >> >>  On 21.12.2015 at 8:50pm, <JBeulich@suse.com> wrote:
> >> >> >>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
> >> >> > On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
> >> >> >> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
> >
> > 1.
> >> >> > IMO, When VT-d is enabled, but is not working correct. These
> >> >> > PCI-e devices
> >> >> > (Disks/NICs..) DMA/Interrupt behaviors are not predictable.
> >> >> > Assumed that, VT-d is effectively not in use for domains without
> >> >> > PT device, while at least the virtualization infrastructure is not trusted.
> >
> > 2.
> >> >> > IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single
> >> >> > domain behavior, it is a Hypervisor and infrastructure issue.
> >> >> > ATS device's Device-TLB flush is a single domain issue.
> >
> > One quick question,
> > Jan, do you agreed the above 2 descriptions?
> 
> I agree, but (see also Andrew's earlier reply) don't take them as an excuse to
> crash Xen upon flush failures. Please accept that the general policy has to be to
> handle errors with as narrow an impact as possible.
>

That maybe the gap between us. It is really an issue that require to crash Xen.

I think we are on the same page for Device-TLB flush issue.
Could you share your idea how to deal with VT-d (IEC/Context/Iotlb) flush issue?  Thanks.

Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Jan Beulich]

>>> On 21.12.15 at 15:31, <quan.xu@intel.com> wrote:
>>  On 21.12.2015 at 10:16pm, <JBeulich@suse.com> wrote:
>> >>> On 21.12.15 at 15:08, <quan.xu@intel.com> wrote:
>> >>  On 21.12.2015 at 9:23pm, <JBeulich@suse.com> wrote:
>> >> >>> On 21.12.15 at 14:08, <quan.xu@intel.com> wrote:
>> >> >>  On 21.12.2015 at 8:50pm, <JBeulich@suse.com> wrote:
>> >> >> >>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
>> >> >> > On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
>> >> >> >> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
>> >
>> > 1.
>> >> >> > IMO, When VT-d is enabled, but is not working correct. These
>> >> >> > PCI-e devices
>> >> >> > (Disks/NICs..) DMA/Interrupt behaviors are not predictable.
>> >> >> > Assumed that, VT-d is effectively not in use for domains without
>> >> >> > PT device, while at least the virtualization infrastructure is not 
> trusted.
>> >
>> > 2.
>> >> >> > IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single
>> >> >> > domain behavior, it is a Hypervisor and infrastructure issue.
>> >> >> > ATS device's Device-TLB flush is a single domain issue.
>> >
>> > One quick question,
>> > Jan, do you agreed the above 2 descriptions?
>> 
>> I agree, but (see also Andrew's earlier reply) don't take them as an excuse to
>> crash Xen upon flush failures. Please accept that the general policy has to be to
>> handle errors with as narrow an impact as possible.
>>
> 
> That maybe the gap between us. It is really an issue that require to crash 
> Xen.
> 
> I think we are on the same page for Device-TLB flush issue.
> Could you share your idea how to deal with VT-d (IEC/Context/Iotlb) flush 
> issue?  Thanks.

I think this was sufficiently outlined before, by both Andrew and me.
I'm sorry, but I'm not willing to start over with the discussion.

Jan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

>On 21.12.2015 at 10:53pm, <JBeulich@suse.com> wrote:
> >>> On 21.12.15 at 15:31, <quan.xu@intel.com> wrote:
> >>  On 21.12.2015 at 10:16pm, <JBeulich@suse.com> wrote:
> >> >>> On 21.12.15 at 15:08, <quan.xu@intel.com> wrote:
> >> >>  On 21.12.2015 at 9:23pm, <JBeulich@suse.com> wrote:
> >> >> >>> On 21.12.15 at 14:08, <quan.xu@intel.com> wrote:
> >> >> >>  On 21.12.2015 at 8:50pm, <JBeulich@suse.com> wrote:
> >> >> >> >>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
> >> >> >> > On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
> >> >> >> >> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
> >> >
> >> > 1.
> >> >> >> > IMO, When VT-d is enabled, but is not working correct. These
> >> >> >> > PCI-e devices
> >> >> >> > (Disks/NICs..) DMA/Interrupt behaviors are not predictable.
> >> >> >> > Assumed that, VT-d is effectively not in use for domains
> >> >> >> > without PT device, while at least the virtualization
> >> >> >> > infrastructure is not
> > trusted.
> >> >
> >> > 2.
> >> >> >> > IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single
> >> >> >> > domain behavior, it is a Hypervisor and infrastructure issue.
> >> >> >> > ATS device's Device-TLB flush is a single domain issue.
> >> >
> >> > One quick question,
> >> > Jan, do you agreed the above 2 descriptions?
> >>
> >> I agree, but (see also Andrew's earlier reply) don't take them as an
> >> excuse to crash Xen upon flush failures. Please accept that the
> >> general policy has to be to handle errors with as narrow an impact as
> possible.
> >>
> >
> > That maybe the gap between us. It is really an issue that require to
> > crash Xen.
> >
> > I think we are on the same page for Device-TLB flush issue.
> > Could you share your idea how to deal with VT-d (IEC/Context/Iotlb)
> > flush issue?  Thanks.
> 
> I think this was sufficiently outlined before, by both Andrew and me.
> I'm sorry, but I'm not willing to start over with the discussion.
> 

Jan, Never mind.. I understand that.
But could you forward the result to me? when you are available. 
I am really not in the previous thread and I can't find in in my Xen maillist archive. Thanks.

Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, December 21, 2015 9:23 PM
> To: Xu, Quan <quan.xu@intel.com>
> Cc: 'andrew.cooper3@citrix.com' <andrew.cooper3@citrix.com>;
> 'george.dunlap@eu.citrix.com' <george.dunlap@eu.citrix.com>; Wu, Feng
> <feng.wu@intel.com>; Nakajima, Jun <jun.nakajima@intel.com>; Tian, Kevin
> <kevin.tian@intel.com>; 'xen-devel@lists.xen.org' <xen-devel@lists.xen.org>;
> 'keir@xen.org' <keir@xen.org>; 'tim@xen.org' <tim@xen.org>
> Subject: RE: [Xen-devel] [PATCH v3 0/2] VT-d flush issue
> 
> >>> On 21.12.15 at 14:08, <quan.xu@intel.com> wrote:
> >>  On 21.12.2015 at 8:50pm, <JBeulich@suse.com> wrote:
> >> >>> On 21.12.15 at 13:28, <quan.xu@intel.com> wrote:
> >> > On 21.12.2015 at 7:47pm, <JBeulich@suse.com> wrote:
> >> >> >>> On 20.12.15 at 14:57, <quan.xu@intel.com> wrote:
> >> >> > 2. If VT-d is bug, does the hardware_domain continue to work with
> >> >> > PCIe Devices / DRAM well with DMA remapping error?
> >> >> >    I think it is no. furthermore, i think VMM can NOT run a normal
> >> >> > HVM domain without device-passthrough.
> >> >>
> >> >> In addition to what Andrew said - VT-d is effectively not in use for
> >> >> domains without PT device.
> >> >
> >> > IMO, When VT-d is enabled, but is not working correct. These PCI-e
> >> > devices
> >> > (Disks/NICs..) DMA/Interrupt behaviors are not predictable.
> >> > Assumed that, VT-d is effectively not in use for domains without PT
> >> > device, while at least the virtualization infrastructure is not trusted.
> >> > I think it is also not secure to run PV domains.
> >> >
> >> >> Impacting all such domains by crashing the hypervisor just because
> >> >> (in the extreme case) a single domain with PT devices exhibited a
> >> >> flush issue is a no-go imo.
> >> >>
> >> >
> >> > IMO, a VT-d (IEC/Context/Iotlb) flush issue is not a single domain
> >> > behavior, it is a Hypervisor and infrastructure issue.
> >> > ATS device's Device-TLB flush is a single domain issue.
> >> > Back to our original goal, my patch set is for ATS flush issue. right?
> >>
> >> You mean you don't like this entailing clean up of other code?
> >
> >  Jan, for ARM/AMD, I really have no knowledge to fix it. and I have no
> >  ARM/AMD hardware to verify it. if I need to fix these common part of
> > INTEL/ARM/AMD, I think I need to make
> >  Xen compile correct and not to destroy the logic.
> 
> You indeed aren't expected to fix AMD or ARM code, but it may be
> necessary to adjust that code to make error propagation work.
> 
> >> I'm sorry, but I'm
> >> afraid you won't get away without - perhaps the VT-d maintainers could help
> >> here, but in the end you have to face that it was mainly Intel people who
> >> introduced the code which now needs fixing up, so I consider it not exactly
> >> unfair for you (as a
> >> company) to do this work.
> >>
> >
> > Furthermore, I found out that
> >      if IEC/Iotlb/Context flush error, then panic.
> >      Else if device-tlb flush error, we'll hide the target ATS device and
> > kill the domain owning this ATS device. If impacted domain is hardware
> > domain, just throw out a warning.
> >
> >      Then, it is fine to _not_check all the way up the device-tlb flush call
> > trees( maybe it is our next topic of discussion).
> 
> I don't follow - this sounds more or less like the model you've been
> following in past versions, yet it was that which prompted the
> request to properly propagate errors.

Maybe, there are still some misunderstanding about your expectation.
Let me summarize it here.

After Quan's patch-set, there are two types of error code:
- -EOPNOTSUPP
Now we only support and use software way to synchronize the invalidation,
if someone calls queue_invalidate_wait() and passes sw with 0, then
-EOPNOTSUPP is returned (Though this cannot happen in real world, since 
queue_invalidate_wait() is called only in one place and 1 is passed in to 'sw').
So I am not sure what should we do for this return value, if we really get
that return value, it means the flush is not actually executed, so the iommu
state is incorrect, the data is inconsistent. Do you think what should we do
for this case?

- -ETIMEDOUT
For this case, Quan has elaborate a lot, IIUIC, the main gap between you
and Quan is you think the error code should be propagated to the up caller,
while in Quan's implementation, he deals with this error in invalidate_timeout()
and device_tlb_invalidate_timeout(), hence no need to propagated it to
up called, since the handling policy will crash the domain, so we don't think
propagated error code is needed. Even we propagate it, the up caller still
doesn't need to do anything for it.

Quan, please correct me if I am wrong.

Thanks,
Feng

Re: [PATCH v3 0/2] VT-d flush issue

[Jan Beulich]

>>> On 22.12.15 at 08:40, <feng.wu@intel.com> wrote:
> Maybe, there are still some misunderstanding about your expectation.
> Let me summarize it here.
> 
> After Quan's patch-set, there are two types of error code:
> - -EOPNOTSUPP
> Now we only support and use software way to synchronize the invalidation,
> if someone calls queue_invalidate_wait() and passes sw with 0, then
> -EOPNOTSUPP is returned (Though this cannot happen in real world, since 
> queue_invalidate_wait() is called only in one place and 1 is passed in to 'sw').
> So I am not sure what should we do for this return value, if we really get
> that return value, it means the flush is not actually executed, so the iommu
> state is incorrect, the data is inconsistent. Do you think what should we do
> for this case?

Since seeing this error would be a software bug, BUG() or ASSERT()
are fine to handle this specific case, if need be.

> - -ETIMEDOUT
> For this case, Quan has elaborate a lot, IIUIC, the main gap between you
> and Quan is you think the error code should be propagated to the up caller,
> while in Quan's implementation, he deals with this error in 
> invalidate_timeout()
> and device_tlb_invalidate_timeout(), hence no need to propagated it to
> up called, since the handling policy will crash the domain, so we don't think
> propagated error code is needed. Even we propagate it, the up caller still
> doesn't need to do anything for it.

"Handling" an error by e.g. domain_crash() doesn't mean you don't
need to also modify (or at the very least inspect) callers: They may
continue doing things _assuming_ success. Of course you don't
need to domain_crash() at all layers. But errors from lower layers
should, at least in most ordinary cases, lead to higher layers bailing
instead of continuing.

Jan

Re: [PATCH v3 0/2] VT-d flush issue

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, December 22, 2015 4:01 PM
> To: Wu, Feng <feng.wu@intel.com>; Xu, Quan <quan.xu@intel.com>
> Cc: 'andrew.cooper3@citrix.com' <andrew.cooper3@citrix.com>;
> 'george.dunlap@eu.citrix.com' <george.dunlap@eu.citrix.com>; Nakajima, Jun
> <jun.nakajima@intel.com>; Tian, Kevin <kevin.tian@intel.com>; 'xen-
> devel@lists.xen.org' <xen-devel@lists.xen.org>; 'keir@xen.org' <keir@xen.org>;
> 'tim@xen.org' <tim@xen.org>
> Subject: RE: [Xen-devel] [PATCH v3 0/2] VT-d flush issue
> 
> >>> On 22.12.15 at 08:40, <feng.wu@intel.com> wrote:
> > Maybe, there are still some misunderstanding about your expectation.
> > Let me summarize it here.
> >
> > After Quan's patch-set, there are two types of error code:
> > - -EOPNOTSUPP
> > Now we only support and use software way to synchronize the invalidation,
> > if someone calls queue_invalidate_wait() and passes sw with 0, then
> > -EOPNOTSUPP is returned (Though this cannot happen in real world, since
> > queue_invalidate_wait() is called only in one place and 1 is passed in to 'sw').
> > So I am not sure what should we do for this return value, if we really get
> > that return value, it means the flush is not actually executed, so the iommu
> > state is incorrect, the data is inconsistent. Do you think what should we do
> > for this case?
> 
> Since seeing this error would be a software bug, BUG() or ASSERT()
> are fine to handle this specific case, if need be.
> 
> > - -ETIMEDOUT
> > For this case, Quan has elaborate a lot, IIUIC, the main gap between you
> > and Quan is you think the error code should be propagated to the up caller,
> > while in Quan's implementation, he deals with this error in
> > invalidate_timeout()
> > and device_tlb_invalidate_timeout(), hence no need to propagated it to
> > up called, since the handling policy will crash the domain, so we don't think
> > propagated error code is needed. Even we propagate it, the up caller still
> > doesn't need to do anything for it.
> 
> "Handling" an error by e.g. domain_crash() doesn't mean you don't
> need to also modify (or at the very least inspect) callers: They may
> continue doing things _assuming_ success. Of course you don't
> need to domain_crash() at all layers. But errors from lower layers
> should, at least in most ordinary cases, lead to higher layers bailing
> instead of continuing.

So there are two questions:
1. Just confirmed with Quan, for IEC/context/iotlb flush timeout, the policy
will be Xen panic. Do you agree with this policy? If you do, we don't need to
pass the error code to the caller, right? If you don't, we may need more
discussion about how the handle this case first before anything else.
2. For Device iotlb flush, the current policy is to crash the domain, in this case
we need to check case by case whether the caller need to handle something
if timeout is encountered, right? If needed, we need do something there, if
not needed, we can just let it be.

Thanks,
Feng

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

>On 22.12.2015 at 4:01pm <JBeulich@suse.com> wrote:
> >>> On 22.12.15 at 08:40, <feng.wu@intel.com> wrote:
> > Maybe, there are still some misunderstanding about your expectation.
> > Let me summarize it here.
> >
> > After Quan's patch-set, there are two types of error code:
> > - -EOPNOTSUPP
> > Now we only support and use software way to synchronize the
> > invalidation, if someone calls queue_invalidate_wait() and passes sw
> > with 0, then -EOPNOTSUPP is returned (Though this cannot happen in
> > real world, since
> > queue_invalidate_wait() is called only in one place and 1 is passed in to 'sw').
> > So I am not sure what should we do for this return value, if we really
> > get that return value, it means the flush is not actually executed, so
> > the iommu state is incorrect, the data is inconsistent. Do you think
> > what should we do for this case?
> 
> Since seeing this error would be a software bug, BUG() or ASSERT() are fine to
> handle this specific case, if need be.
> 
> > - -ETIMEDOUT
> > For this case, Quan has elaborate a lot, IIUIC, the main gap between
> > you and Quan is you think the error code should be propagated to the
> > up caller, while in Quan's implementation, he deals with this error in
> > invalidate_timeout()
> > and device_tlb_invalidate_timeout(), hence no need to propagated it to
> > up called, since the handling policy will crash the domain, so we
> > don't think propagated error code is needed. Even we propagate it, the
> > up caller still doesn't need to do anything for it.
> 
> "Handling" an error by e.g. domain_crash() doesn't mean you don't need to also
> modify (or at the very least inspect) callers: They may continue doing things
> _assuming_ success. Of course you don't need to domain_crash() at all layers.
> But errors from lower layers should, at least in most ordinary cases, lead to
> higher layers bailing instead of continuing.
> 

For Device-TLB flush error, I think we need to propagated error code.
For IEC/iotlb/context flush error, if panic is acceptable, we can ignore the propagated error code. BTW, it is very challenge / tricky to handle all
Of error, and some error is unrecoverable. As mentioned, it looks like rewriting Xen hypervisor.

For -EOPNOTSUPP, we can print warning message. If it supports interrupt method, we can return 0 in queue_invalidate_wait().

Feng, thanks for your update!

-Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Jan Beulich]

>>> On 22.12.15 at 09:10, <feng.wu@intel.com> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Tuesday, December 22, 2015 4:01 PM
>> To: Wu, Feng <feng.wu@intel.com>; Xu, Quan <quan.xu@intel.com>
>> Cc: 'andrew.cooper3@citrix.com' <andrew.cooper3@citrix.com>;
>> 'george.dunlap@eu.citrix.com' <george.dunlap@eu.citrix.com>; Nakajima, Jun
>> <jun.nakajima@intel.com>; Tian, Kevin <kevin.tian@intel.com>; 'xen-
>> devel@lists.xen.org' <xen-devel@lists.xen.org>; 'keir@xen.org' <keir@xen.org>;
>> 'tim@xen.org' <tim@xen.org>
>> Subject: RE: [Xen-devel] [PATCH v3 0/2] VT-d flush issue
>> 
>> >>> On 22.12.15 at 08:40, <feng.wu@intel.com> wrote:
>> > Maybe, there are still some misunderstanding about your expectation.
>> > Let me summarize it here.
>> >
>> > After Quan's patch-set, there are two types of error code:
>> > - -EOPNOTSUPP
>> > Now we only support and use software way to synchronize the invalidation,
>> > if someone calls queue_invalidate_wait() and passes sw with 0, then
>> > -EOPNOTSUPP is returned (Though this cannot happen in real world, since
>> > queue_invalidate_wait() is called only in one place and 1 is passed in to 
> 'sw').
>> > So I am not sure what should we do for this return value, if we really get
>> > that return value, it means the flush is not actually executed, so the 
> iommu
>> > state is incorrect, the data is inconsistent. Do you think what should we 
> do
>> > for this case?
>> 
>> Since seeing this error would be a software bug, BUG() or ASSERT()
>> are fine to handle this specific case, if need be.
>> 
>> > - -ETIMEDOUT
>> > For this case, Quan has elaborate a lot, IIUIC, the main gap between you
>> > and Quan is you think the error code should be propagated to the up caller,
>> > while in Quan's implementation, he deals with this error in
>> > invalidate_timeout()
>> > and device_tlb_invalidate_timeout(), hence no need to propagated it to
>> > up called, since the handling policy will crash the domain, so we don't 
> think
>> > propagated error code is needed. Even we propagate it, the up caller still
>> > doesn't need to do anything for it.
>> 
>> "Handling" an error by e.g. domain_crash() doesn't mean you don't
>> need to also modify (or at the very least inspect) callers: They may
>> continue doing things _assuming_ success. Of course you don't
>> need to domain_crash() at all layers. But errors from lower layers
>> should, at least in most ordinary cases, lead to higher layers bailing
>> instead of continuing.
> 
> So there are two questions:
> 1. Just confirmed with Quan, for IEC/context/iotlb flush timeout, the policy
> will be Xen panic. Do you agree with this policy? If you do, we don't need 
> to
> pass the error code to the caller, right? If you don't, we may need more
> discussion about how the handle this case first before anything else.

Just to repeat several earlier statements: BUG() and panic() ought
to be avoided wherever possible (i.e. including here).

> 2. For Device iotlb flush, the current policy is to crash the domain, in 
> this case
> we need to check case by case whether the caller need to handle something
> if timeout is encountered, right? If needed, we need do something there, if
> not needed, we can just let it be.

"Just let it be" is wrong (and is what led to the bad state the code is
in right now in this regard). I gave advice for the direction before:
Add __must_check. This will make the compiler point out to you
where changes are needed.

Jan

Re: [PATCH v3 0/2] VT-d flush issue

[Jan Beulich]

>>> On 22.12.15 at 09:10, <quan.xu@intel.com> wrote:
> For Device-TLB flush error, I think we need to propagated error code.
> For IEC/iotlb/context flush error, if panic is acceptable, we can ignore the 
> propagated error code. BTW, it is very challenge / tricky to handle all
> Of error, and some error is unrecoverable. As mentioned, it looks like 
> rewriting Xen hypervisor.

We're moving in circles. In particular I don't believe this last sentence.
Re-writing many parts of the hypervisor would be required is you
were to return the error to the guest (which technically isn't possible
in many case afaict). Having crashed the guest, you don't need to be
concerned about unrolling previous (partially completed) operations,
so I don't think it's this much of a rewrite.

And just to be clear - I hope you recall that the current approach
taken to the flush issue is already a compromise far away from
where we initially meant the code to move, and it was always
clear that the bad error handling state the code is in is going to
get in the way of this being a simple fix. It's sad that the people
originally having introduced that code can't be held responsible
for fixing this up, but that's a situation we find ourselves in all
the time.

Jan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

> On December 22, 2015 4:27pm, <JBeulich@suse.com> wrote:
> >>> On 22.12.15 at 09:10, <quan.xu@intel.com> wrote:
> > For Device-TLB flush error, I think we need to propagated error code.
> > For IEC/iotlb/context flush error, if panic is acceptable, we can
> > ignore the propagated error code. BTW, it is very challenge / tricky
> > to handle all Of error, and some error is unrecoverable. As mentioned,
> > it looks like rewriting Xen hypervisor.
> 
> We're moving in circles. In particular I don't believe this last sentence.
> Re-writing many parts of the hypervisor would be required is you were to return
> the error to the guest (which technically isn't possible in many case afaict).
> Having crashed the guest, you don't need to be concerned about unrolling
> previous (partially completed) operations, so I don't think it's this much of a
> rewrite.
> 
> And just to be clear - I hope you recall that the current approach taken to the
> flush issue is already a compromise far away from where we initially meant the
> code to move, and it was always clear that the bad error handling state the
> code is in is going to get in the way of this being a simple fix. It's sad that the
> people originally having introduced that code can't be held responsible for fixing
> this up, but that's a situation we find ourselves in all the time.
> 
> Jan

Jan,
Let's finish our discussion. I accept your idea. But I need to separate it into 3 patch set (It is complicated for me, sometime it makes me crash.):
   Patch set 1:  Device-TLB/iotlb flush error. (send out this week)
   Patch set 2:  context flush error. (need 2 ~ 3 weeks)
   Patch set 3:  iec flush error. (need 3 ~ 4 weeks)

If it is acceptable, we can discuss in detail one by one..

Thanks
-Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Jan Beulich]

>>> On 22.12.15 at 09:43, <quan.xu@intel.com> wrote:
> Let's finish our discussion. I accept your idea. But I need to separate it 
> into 3 patch set (It is complicated for me, sometime it makes me crash.):
>    Patch set 1:  Device-TLB/iotlb flush error. (send out this week)
>    Patch set 2:  context flush error. (need 2 ~ 3 weeks)
>    Patch set 3:  iec flush error. (need 3 ~ 4 weeks)
> 
> If it is acceptable, we can discuss in detail one by one..

Splitting up is of course acceptable.

Jan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

> On 22.12.2015 at 5:09pm, <JBeulich@suse.com> wrote:
> >>> On 22.12.15 at 09:43, <quan.xu@intel.com> wrote:
> > Let's finish our discussion. I accept your idea. But I need to
> > separate it into 3 patch set (It is complicated for me, sometime it makes me
> crash.):
> >    Patch set 1:  Device-TLB/iotlb flush error. (send out this week)
> >    Patch set 2:  context flush error. (need 2 ~ 3 weeks)
> >    Patch set 3:  iec flush error. (need 3 ~ 4 weeks)
> >
> > If it is acceptable, we can discuss in detail one by one..
> 
> Splitting up is of course acceptable.
> 

Thanks Jan. I am getting started to Patch set 1. 

Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

> On 22.12.2015 at 5:09pm, <JBeulich@suse.com> wrote:
> >>> On 22.12.15 at 09:43, <quan.xu@intel.com> wrote:
> > Let's finish our discussion. I accept your idea. But I need to
> > separate it into 3 patch set (It is complicated for me, sometime it makes me
> crash.):
> >    Patch set 1:  Device-TLB/iotlb flush error. (send out this week)
> >    Patch set 2:  context flush error. (need 2 ~ 3 weeks)
> >    Patch set 3:  iec flush error. (need 3 ~ 4 weeks)
> >
> > If it is acceptable, we can discuss in detail one by one..
> 
> Splitting up is of course acceptable.

Jan,
   Just update the combination,

   Patch set 1:  Device-TLB flush error. (send out this week)
   Patch set 2:  context/iotlb flush error. (need 2 ~ 3 weeks)
   Patch set 3:  iec flush error. (need 3 ~ 4 weeks)

-Quan

Re: [PATCH v3 0/2] VT-d flush issue

[Tian, Kevin]

> From: Xu, Quan
> Sent: Tuesday, December 22, 2015 6:26 PM
> 
> > On 22.12.2015 at 5:09pm, <JBeulich@suse.com> wrote:
> > >>> On 22.12.15 at 09:43, <quan.xu@intel.com> wrote:
> > > Let's finish our discussion. I accept your idea. But I need to
> > > separate it into 3 patch set (It is complicated for me, sometime it makes me
> > crash.):
> > >    Patch set 1:  Device-TLB/iotlb flush error. (send out this week)
> > >    Patch set 2:  context flush error. (need 2 ~ 3 weeks)
> > >    Patch set 3:  iec flush error. (need 3 ~ 4 weeks)
> > >
> > > If it is acceptable, we can discuss in detail one by one..
> >
> > Splitting up is of course acceptable.
> 
> Jan,
>    Just update the combination,
> 
>    Patch set 1:  Device-TLB flush error. (send out this week)
>    Patch set 2:  context/iotlb flush error. (need 2 ~ 3 weeks)
>    Patch set 3:  iec flush error. (need 3 ~ 4 weeks)
> 
> -Quan

Sorry being late to this discussion. Thanks Jan/Andrew's comments
for overall gap in VT-d error handling. Intel is definitely committed 
to continue improve current code quality, but let's do things one-by-one.

Above is a good split. Let's start from there.

Thanks
Kevin

Re: [PATCH v3 0/2] VT-d flush issue

[Xu, Quan]

>On 12.12.2015 at 9:22pm, <quan.xu@intel.com> wrote:
> This patches are based on Kevin Tian's previous discussion 'Revisit VT-d
> asynchronous flush issue'.
> Fix current timeout concern and also allow limited ATS support in a light way:


> 2. Fix vt-d flush timeout issue.
> 
>     If IOTLB/Context/IETC flush is timeout, we should think all devices under
> this IOMMU cannot function correctly.
>     So for each device under this IOMMU we'll mark it as unassignable and kill
> the domain owning the device.
> 

Any comment for this point?
Instead of panic, is it enough?

-Quan



>     If Device-TLB flush is timeout, we'll mark the target ATS device as
> unassignable and kill the domain owning
>     this device.
> 
>     If impacted domain is hardware domain, just throw out a warning. It's an
> open here whether we want to kill
>     hardware domain (or directly panic hypervisor). Comments are welcomed.
> 
>     Device marked as unassignable will be disallowed to be further assigned to
> any domain.

[PATCH v3 0/2] VT-d flush issue

[Quan Xu]

This patches are based on Kevin Tian's previous discussion 'Revisit VT-d asynchronous flush issue'.
Fix current timeout concern and also allow limited ATS support in a light way:

1. Reduce spin timeout to 1ms, which can be boot-time changed with 'iommu_qi_timeout_ms'.
   For example:
           multiboot /boot/xen.gz ats=1 iommu_qi_timeout_ms=100

2. Fix vt-d flush timeout issue.

    If IOTLB/Context/IETC flush is timeout, we should think all devices under this IOMMU cannot function correctly.
    So for each device under this IOMMU we'll mark it as unassignable and kill the domain owning the device.

    If Device-TLB flush is timeout, we'll mark the target ATS device as unassignable and kill the domain owning
    this device.

    If impacted domain is hardware domain, just throw out a warning. It's an open here whether we want to kill
    hardware domain (or directly panic hypervisor). Comments are welcomed.

    Device marked as unassignable will be disallowed to be further assigned to any domain.

--

 * DMAR_OPERATION_TIMEOUT should be also chopped down to a low number of milliseconds.
   As Kevin Tian mentioned in 'Revisit VT-d asynchronous flush issue', We also confirmed with hardware team
   that 1ms is large enough for IOMMU internal flush. So I can change DMAR_OPERATION_TIMEOUT from 1000 ms to 1 ms.

   IOMMU_WAIT_OP() is only for VT-d registers read/write, and there is also a panic. We need a further discussion
   whether or how to remove this panic in next patch set. 


 * Kevin Tian did basic functional review.

--Changes in v3:
   1. Once found you can break the for loop immediately since BDF is unique.

   2. Separate invalidate_timeout(struct iommu *iommu, int type, u16 did, u16 seg, u8 bus, u8 devfn)
      into invalidate_timeout(struct iommu *iommu) and device_tlb_invalidate_timeout(struct iommu *iommu,
      u16 did, u16 seg, u8 bus, u8 devfn).
      invalidate_timeout() is for iotlb/iec/context flush error.
      device_tlb_invalidate_timeout() is for Device-TLB flush error. 
      then ignore these INVALID_* parameters.

   3. Change macros(mark_pdev_unassignable/IS_PDEV_UNASSIGNABLE) into static inline functions.


Quan Xu (2):
  VT-d: Reduce spin timeout to 1ms, which can be boot-time changed.
  VT-d: Fix vt-d flush timeout issue.

 xen/drivers/passthrough/vtd/extern.h  |  5 ++
 xen/drivers/passthrough/vtd/iommu.c   |  6 +++
 xen/drivers/passthrough/vtd/qinval.c  | 89 +++++++++++++++++++++++++++++++++--
 xen/drivers/passthrough/vtd/x86/ats.c | 16 +++++++
 xen/include/xen/pci.h                 | 11 +++++
 5 files changed, 123 insertions(+), 4 deletions(-)

-- 
1.8.1.2