Re: [PATCH v2 2/2] VT-d: Fix vt-d flush timeout issue.

["Xu, Quan" <quan.xu@intel.com>]

On 11.12.2015 at 6:05pm, <JBeulich@suse.com> wrote:
> >>> On 11.12.15 at 09:01, <quan.xu@intel.com> wrote:
> > On 11.12.2015 at 3:28pm, <Tian, Kevin> wrote:
> >> > From: Xu, Quan
> >> > Sent: Thursday, December 10, 2015 5:33 PM
> >> >
> >> > 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.
> >> >
> >> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >> > ---
> >> [...]
> >> > diff --git a/xen/drivers/passthrough/vtd/iommu.h
> >> > b/xen/drivers/passthrough/vtd/iommu.h
> >> > index ac71ed1..c3beaa6 100644
> >> > --- a/xen/drivers/passthrough/vtd/iommu.h
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.h
> >> > @@ -452,6 +452,11 @@ struct qinval_entry {
> >> >
> >> >  #define RESERVED_VAL        0
> >> >
> >> > +#define INVALID_DID    ((u16)~0)
> >> > +#define INVALID_SEG    ((u16)~0)
> >> > +#define INVALID_BUS    ((u8)~0)
> >> > +#define INVALID_DEVFN  ((u8)~0)
> >> > +
> >>
> >> Are those invalid values defined by specification?
> >  This is not defined by specification.
> >
> >>Or if they are software
> >> defined, does related mgmt. code guarantee that they won't be allocated?
> >>
> >
> > As similar as the other Xen code, it defined invalid value with "~0".
> > Such
> > as:
> >           $#define INVALID_MFN (~0UL)
> >           $#define INVALID_GFN (~0UL)
> >           .etc
> >
> > Code can't not guarantee that won't be allocated, but it can guarantee
> > it will not be used when it is INVALID_*.
> > Any idea, how to indicate that the value is invalid?
> 
> Some other means is needed (be creative). Comparing with INVALID_{MFN,GFN}
> is bogus, since frame numbers truly can't reach this big a value (there being just
> 52 bits in physical addresses, i.e. 40 bits in a frame number).

Jan, thanks for your comments.
I think I can't use INVALID_* in my patch any more. If I can separate invalidate_timeout() into 2
Functions. then I can ignore these INVALID_* parameters.

i.e. 
separate INVALID_* parameters. ignore these INVALID_* parameters.
void 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.

Right?

Quan