Message ID | 1478603064-32562-2-git-send-email-bd.aviv@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 2016年11月08日 19:04, Aviv B.D wrote: > From: "Aviv Ben-David" <bd.aviv@gmail.com> > > This capability asks the guest to invalidate cache before each map operation. > We can use this invalidation to trap map operations in the hypervisor. Hi: Like I've asked twice in the past, I want to know why don't you cache translation faults as what spec required (especially this is a guest visible behavior)? Btw, please cc me on posting future versions. Thanks > > Signed-off-by: Aviv Ben-David <bd.aviv@gmail.com> > --- > hw/i386/intel_iommu.c | 5 +++++ > hw/i386/intel_iommu_internal.h | 1 + > include/hw/i386/intel_iommu.h | 2 ++ > 3 files changed, 8 insertions(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 1655a65..834887f 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -2017,6 +2017,7 @@ static Property vtd_properties[] = { > DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim, > ON_OFF_AUTO_AUTO), > DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false), > + DEFINE_PROP_BOOL("cache-mode", IntelIOMMUState, cache_mode_enabled, FALSE), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -2391,6 +2392,10 @@ static void vtd_init(IntelIOMMUState *s) > assert(s->intr_eim != ON_OFF_AUTO_AUTO); > } > > + if (s->cache_mode_enabled) { > + s->cap |= VTD_CAP_CM; > + } > + > vtd_reset_context_cache(s); > vtd_reset_iotlb(s); > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > index 0829a50..35d9f3a 100644 > --- a/hw/i386/intel_iommu_internal.h > +++ b/hw/i386/intel_iommu_internal.h > @@ -201,6 +201,7 @@ > #define VTD_CAP_MAMV (VTD_MAMV << 48) > #define VTD_CAP_PSI (1ULL << 39) > #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) > +#define VTD_CAP_CM (1ULL << 7) > > /* Supported Adjusted Guest Address Widths */ > #define VTD_CAP_SAGAW_SHIFT 8 > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 1989c1e..42d293f 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -258,6 +258,8 @@ struct IntelIOMMUState { > uint8_t womask[DMAR_REG_SIZE]; /* WO (write only - read returns 0) */ > uint32_t version; > > + bool cache_mode_enabled; /* RO - is cap CM enabled? */ > + > dma_addr_t root; /* Current root table pointer */ > bool root_extended; /* Type of root table (extended or not) */ > bool dmar_enabled; /* Set if DMA remapping is enabled */
On Wed, Nov 09, 2016 at 03:28:02PM +0800, Jason Wang wrote: > > > On 2016年11月08日 19:04, Aviv B.D wrote: > > From: "Aviv Ben-David" <bd.aviv@gmail.com> > > > > This capability asks the guest to invalidate cache before each map operation. > > We can use this invalidation to trap map operations in the hypervisor. > > Hi: > > Like I've asked twice in the past, I want to know why don't you cache > translation faults as what spec required (especially this is a guest visible > behavior)? > > Btw, please cc me on posting future versions. > > Thanks Caching isn't guest visible. Spec just says you *can* cache, not that you must. > > > > Signed-off-by: Aviv Ben-David <bd.aviv@gmail.com> > > --- > > hw/i386/intel_iommu.c | 5 +++++ > > hw/i386/intel_iommu_internal.h | 1 + > > include/hw/i386/intel_iommu.h | 2 ++ > > 3 files changed, 8 insertions(+) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index 1655a65..834887f 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -2017,6 +2017,7 @@ static Property vtd_properties[] = { > > DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim, > > ON_OFF_AUTO_AUTO), > > DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false), > > + DEFINE_PROP_BOOL("cache-mode", IntelIOMMUState, cache_mode_enabled, FALSE), > > DEFINE_PROP_END_OF_LIST(), > > }; > > @@ -2391,6 +2392,10 @@ static void vtd_init(IntelIOMMUState *s) > > assert(s->intr_eim != ON_OFF_AUTO_AUTO); > > } > > + if (s->cache_mode_enabled) { > > + s->cap |= VTD_CAP_CM; > > + } > > + > > vtd_reset_context_cache(s); > > vtd_reset_iotlb(s); > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > > index 0829a50..35d9f3a 100644 > > --- a/hw/i386/intel_iommu_internal.h > > +++ b/hw/i386/intel_iommu_internal.h > > @@ -201,6 +201,7 @@ > > #define VTD_CAP_MAMV (VTD_MAMV << 48) > > #define VTD_CAP_PSI (1ULL << 39) > > #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) > > +#define VTD_CAP_CM (1ULL << 7) > > /* Supported Adjusted Guest Address Widths */ > > #define VTD_CAP_SAGAW_SHIFT 8 > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > > index 1989c1e..42d293f 100644 > > --- a/include/hw/i386/intel_iommu.h > > +++ b/include/hw/i386/intel_iommu.h > > @@ -258,6 +258,8 @@ struct IntelIOMMUState { > > uint8_t womask[DMAR_REG_SIZE]; /* WO (write only - read returns 0) */ > > uint32_t version; > > + bool cache_mode_enabled; /* RO - is cap CM enabled? */ > > + > > dma_addr_t root; /* Current root table pointer */ > > bool root_extended; /* Type of root table (extended or not) */ > > bool dmar_enabled; /* Set if DMA remapping is enabled */
On 2016年11月10日 06:00, Michael S. Tsirkin wrote: > On Wed, Nov 09, 2016 at 03:28:02PM +0800, Jason Wang wrote: >> > >> > >> >On 2016年11月08日 19:04, Aviv B.D wrote: >>> > >From: "Aviv Ben-David"<bd.aviv@gmail.com> >>> > > >>> > >This capability asks the guest to invalidate cache before each map operation. >>> > >We can use this invalidation to trap map operations in the hypervisor. >> > >> >Hi: >> > >> >Like I've asked twice in the past, I want to know why don't you cache >> >translation faults as what spec required (especially this is a guest visible >> >behavior)? >> > >> >Btw, please cc me on posting future versions. >> > >> >Thanks > Caching isn't guest visible. Seems not, if one fault mapping were cached by IOTLB. Guest can notice this behavior. > Spec just says you*can* cache, > not that you must. > Yes, but what did in this patch is "don't". What I suggest is just a "can", since anyway the IOTLB entries were limited and could be replaced by other. Thanks
On Fri, Nov 11, 2016 at 10:32:42AM +0800, Jason Wang wrote: > > > On 2016年11月10日 06:00, Michael S. Tsirkin wrote: > > On Wed, Nov 09, 2016 at 03:28:02PM +0800, Jason Wang wrote: > > > > > > > > > > > >On 2016年11月08日 19:04, Aviv B.D wrote: > > > > > >From: "Aviv Ben-David"<bd.aviv@gmail.com> > > > > > > > > > > > >This capability asks the guest to invalidate cache before each map operation. > > > > > >We can use this invalidation to trap map operations in the hypervisor. > > > > > > > >Hi: > > > > > > > >Like I've asked twice in the past, I want to know why don't you cache > > > >translation faults as what spec required (especially this is a guest visible > > > >behavior)? > > > > > > > >Btw, please cc me on posting future versions. > > > > > > > >Thanks > > Caching isn't guest visible. > > Seems not, if one fault mapping were cached by IOTLB. Guest can notice this > behavior. Sorry, I don't get what you are saying. > > Spec just says you*can* cache, > > not that you must. > > > > Yes, but what did in this patch is "don't". What I suggest is just a "can", > since anyway the IOTLB entries were limited and could be replaced by other. > > Thanks Have trouble understanding this. Can you given an example of a guest visible difference?
On 2016年11月11日 11:39, Michael S. Tsirkin wrote: > On Fri, Nov 11, 2016 at 10:32:42AM +0800, Jason Wang wrote: >> >> On 2016年11月10日 06:00, Michael S. Tsirkin wrote: >>> On Wed, Nov 09, 2016 at 03:28:02PM +0800, Jason Wang wrote: >>>>> >>>>> On 2016年11月08日 19:04, Aviv B.D wrote: >>>>>>> From: "Aviv Ben-David"<bd.aviv@gmail.com> >>>>>>> >>>>>>> This capability asks the guest to invalidate cache before each map operation. >>>>>>> We can use this invalidation to trap map operations in the hypervisor. >>>>> Hi: >>>>> >>>>> Like I've asked twice in the past, I want to know why don't you cache >>>>> translation faults as what spec required (especially this is a guest visible >>>>> behavior)? >>>>> >>>>> Btw, please cc me on posting future versions. >>>>> >>>>> Thanks >>> Caching isn't guest visible. >> Seems not, if one fault mapping were cached by IOTLB. Guest can notice this >> behavior. > Sorry, I don't get what you are saying. > >>> Spec just says you*can* cache, >>> not that you must. >>> >> Yes, but what did in this patch is "don't". What I suggest is just a "can", >> since anyway the IOTLB entries were limited and could be replaced by other. >> >> Thanks > Have trouble understanding this. Can you given an example of > a guest visible difference? I guess this may do the detection: 1) map iova A to be non-present. 2) invalidate iova A 3) map iova A to addr B 4) access iova A A correct implemented CM may meet fault in step 4, but with this patch, we never.
On Fri, Nov 11, 2016 at 6:15 AM, Jason Wang <jasowang@redhat.com> wrote: > > > On 2016年11月11日 11:39, Michael S. Tsirkin wrote: > >> On Fri, Nov 11, 2016 at 10:32:42AM +0800, Jason Wang wrote: >> >>> >>> On 2016年11月10日 06:00, Michael S. Tsirkin wrote: >>> >>>> On Wed, Nov 09, 2016 at 03:28:02PM +0800, Jason Wang wrote: >>>> >>>>> >>>>>> On 2016年11月08日 19:04, Aviv B.D wrote: >>>>>> >>>>>>> From: "Aviv Ben-David"<bd.aviv@gmail.com> >>>>>>>> >>>>>>>> This capability asks the guest to invalidate cache before each map >>>>>>>> operation. >>>>>>>> We can use this invalidation to trap map operations in the >>>>>>>> hypervisor. >>>>>>>> >>>>>>> Hi: >>>>>> >>>>>> Like I've asked twice in the past, I want to know why don't you cache >>>>>> translation faults as what spec required (especially this is a guest >>>>>> visible >>>>>> behavior)? >>>>>> >>>>>> Btw, please cc me on posting future versions. >>>>>> >>>>>> Thanks >>>>>> >>>>> Caching isn't guest visible. >>>> >>> Seems not, if one fault mapping were cached by IOTLB. Guest can notice >>> this >>> behavior. >>> >> Sorry, I don't get what you are saying. >> >> Spec just says you*can* cache, >>>> not that you must. >>>> >>>> Yes, but what did in this patch is "don't". What I suggest is just a >>> "can", >>> since anyway the IOTLB entries were limited and could be replaced by >>> other. >>> >>> Thanks >>> >> Have trouble understanding this. Can you given an example of >> a guest visible difference? >> > > I guess this may do the detection: > > 1) map iova A to be non-present. > 2) invalidate iova A > 3) map iova A to addr B > 4) access iova A > > A correct implemented CM may meet fault in step 4, but with this patch, we > never. > > By the specification (from intel) section 6.1 (Caching mode) tells that this bit may be present only on virtual machines. So with just this point the guest can detect that it running in a VM of some sort. Additionally, the wording of the specification enable the host to issue a fault it your scenario but, doesn't requiring it. Thanks, Aviv.
On Fri, Nov 11, 2016 at 12:15:48PM +0800, Jason Wang wrote: > > > On 2016年11月11日 11:39, Michael S. Tsirkin wrote: > > On Fri, Nov 11, 2016 at 10:32:42AM +0800, Jason Wang wrote: > > > > > > On 2016年11月10日 06:00, Michael S. Tsirkin wrote: > > > > On Wed, Nov 09, 2016 at 03:28:02PM +0800, Jason Wang wrote: > > > > > > > > > > > > On 2016年11月08日 19:04, Aviv B.D wrote: > > > > > > > > From: "Aviv Ben-David"<bd.aviv@gmail.com> > > > > > > > > > > > > > > > > This capability asks the guest to invalidate cache before each map operation. > > > > > > > > We can use this invalidation to trap map operations in the hypervisor. > > > > > > Hi: > > > > > > > > > > > > Like I've asked twice in the past, I want to know why don't you cache > > > > > > translation faults as what spec required (especially this is a guest visible > > > > > > behavior)? > > > > > > > > > > > > Btw, please cc me on posting future versions. > > > > > > > > > > > > Thanks > > > > Caching isn't guest visible. > > > Seems not, if one fault mapping were cached by IOTLB. Guest can notice this > > > behavior. > > Sorry, I don't get what you are saying. > > > > > > Spec just says you*can* cache, > > > > not that you must. > > > > > > > Yes, but what did in this patch is "don't". What I suggest is just a "can", > > > since anyway the IOTLB entries were limited and could be replaced by other. > > > > > > Thanks > > Have trouble understanding this. Can you given an example of > > a guest visible difference? > > I guess this may do the detection: > > 1) map iova A to be non-present. > 2) invalidate iova A > 3) map iova A to addr B > 4) access iova A > > A correct implemented CM may meet fault in step 4, but with this patch, we > never. I think that IOTLB is free to invalidate entries at any point, so the fault is not guaranteed on bare metal.
On 2016年11月21日 20:41, Aviv B.D. wrote: > > > On Fri, Nov 11, 2016 at 6:15 AM, Jason Wang <jasowang@redhat.com > <mailto:jasowang@redhat.com>> wrote: > > > > On 2016年11月11日 11:39, Michael S. Tsirkin wrote: > > On Fri, Nov 11, 2016 at 10:32:42AM +0800, Jason Wang wrote: > > > On 2016年11月10日 06:00, Michael S. Tsirkin wrote: > > On Wed, Nov 09, 2016 at 03:28:02PM +0800, Jason Wang > wrote: > > > On 2016年11月08日 19:04, Aviv B.D wrote: > > From: "Aviv > Ben-David"<bd.aviv@gmail.com > <mailto:bd.aviv@gmail.com>> > > This capability asks the guest to > invalidate cache before each map > operation. > We can use this invalidation to trap > map operations in the hypervisor. > > Hi: > > Like I've asked twice in the past, I want to > know why don't you cache > translation faults as what spec required > (especially this is a guest visible > behavior)? > > Btw, please cc me on posting future versions. > > Thanks > > Caching isn't guest visible. > > Seems not, if one fault mapping were cached by IOTLB. > Guest can notice this > behavior. > > Sorry, I don't get what you are saying. > > Spec just says you*can* cache, > not that you must. > > Yes, but what did in this patch is "don't". What I suggest > is just a "can", > since anyway the IOTLB entries were limited and could be > replaced by other. > > Thanks > > Have trouble understanding this. Can you given an example of > a guest visible difference? > > > I guess this may do the detection: > > 1) map iova A to be non-present. > 2) invalidate iova A > 3) map iova A to addr B > 4) access iova A > > A correct implemented CM may meet fault in step 4, but with this > patch, we never. > > > By the specification (from intel) section 6.1 (Caching mode) tells > that this bit may be present > only on virtual machines. Yes, but we should also align the behavior to the spec. > So with just this point the guest can detect that it running in > a VM of some sort. Additionally, the wording of the specification > enable the host to issue a fault > it your scenario but, doesn't requiring it. I'm not sure I get the meaning, is there a section that describes your meaning in the spec? Thanks > > Thanks, > Aviv.
On 2016年11月21日 21:35, Michael S. Tsirkin wrote: > On Fri, Nov 11, 2016 at 12:15:48PM +0800, Jason Wang wrote: >> > >> > >> >On 2016年11月11日 11:39, Michael S. Tsirkin wrote: >>> > >On Fri, Nov 11, 2016 at 10:32:42AM +0800, Jason Wang wrote: >>>> > > > >>>> > > >On 2016年11月10日 06:00, Michael S. Tsirkin wrote: >>>>> > > > >On Wed, Nov 09, 2016 at 03:28:02PM +0800, Jason Wang wrote: >>>>>>> > > > > > > >>>>>>> > > > > > >On 2016年11月08日 19:04, Aviv B.D wrote: >>>>>>>>> > > > > > > > >From: "Aviv Ben-David"<bd.aviv@gmail.com> >>>>>>>>> > > > > > > > > >>>>>>>>> > > > > > > > >This capability asks the guest to invalidate cache before each map operation. >>>>>>>>> > > > > > > > >We can use this invalidation to trap map operations in the hypervisor. >>>>>>> > > > > > >Hi: >>>>>>> > > > > > > >>>>>>> > > > > > >Like I've asked twice in the past, I want to know why don't you cache >>>>>>> > > > > > >translation faults as what spec required (especially this is a guest visible >>>>>>> > > > > > >behavior)? >>>>>>> > > > > > > >>>>>>> > > > > > >Btw, please cc me on posting future versions. >>>>>>> > > > > > > >>>>>>> > > > > > >Thanks >>>>> > > > >Caching isn't guest visible. >>>> > > >Seems not, if one fault mapping were cached by IOTLB. Guest can notice this >>>> > > >behavior. >>> > >Sorry, I don't get what you are saying. >>> > > >>>>> > > > >Spec just says you*can* cache, >>>>> > > > >not that you must. >>>>> > > > > >>>> > > >Yes, but what did in this patch is "don't". What I suggest is just a "can", >>>> > > >since anyway the IOTLB entries were limited and could be replaced by other. >>>> > > > >>>> > > >Thanks >>> > >Have trouble understanding this. Can you given an example of >>> > >a guest visible difference? >> > >> >I guess this may do the detection: >> > >> >1) map iova A to be non-present. >> >2) invalidate iova A >> >3) map iova A to addr B >> >4) access iova A >> > >> >A correct implemented CM may meet fault in step 4, but with this patch, we >> >never. > I think that IOTLB is free to invalidate entries at any point, > so the fault is not guaranteed on bare metal. Yes, that's the interesting point. The fault is not guaranteed but conditional. And we have similar issue for IEC. So in conclusion (since I can't find a hardware IOMMU that have CM set): 1) If we don't cache fault conditions, we are still in question that whether it was spec compatible. 2) If we do cache fault conditions, we are 100% sure it was spec compatible. Consider 2) is not complicated, we'd better do it I believe? Thanks
On Tue, Nov 22, 2016 at 6:11 AM, Jason Wang <jasowang@redhat.com> wrote: > > > On 2016年11月21日 21:35, Michael S. Tsirkin wrote: > >> On Fri, Nov 11, 2016 at 12:15:48PM +0800, Jason Wang wrote: >> >>> > >>> > >>> >On 2016年11月11日 11:39, Michael S. Tsirkin wrote: >>> >>>> > >On Fri, Nov 11, 2016 at 10:32:42AM +0800, Jason Wang wrote: >>>> >>>>> > > > >>>>> > > >On 2016年11月10日 06:00, Michael S. Tsirkin wrote: >>>>> >>>>>> > > > >On Wed, Nov 09, 2016 at 03:28:02PM +0800, Jason Wang wrote: >>>>>> >>>>>>> > > > > > > >>>>>>>> > > > > > >On 2016年11月08日 19:04, Aviv B.D wrote: >>>>>>>> >>>>>>>>> > > > > > > > >From: "Aviv Ben-David"<bd.aviv@gmail.com> >>>>>>>>>> > > > > > > > > >>>>>>>>>> > > > > > > > >This capability asks the guest to invalidate cache >>>>>>>>>> before each map operation. >>>>>>>>>> > > > > > > > >We can use this invalidation to trap map >>>>>>>>>> operations in the hypervisor. >>>>>>>>>> >>>>>>>>> > > > > > >Hi: >>>>>>>> > > > > > > >>>>>>>> > > > > > >Like I've asked twice in the past, I want to know why >>>>>>>> don't you cache >>>>>>>> > > > > > >translation faults as what spec required (especially >>>>>>>> this is a guest visible >>>>>>>> > > > > > >behavior)? >>>>>>>> > > > > > > >>>>>>>> > > > > > >Btw, please cc me on posting future versions. >>>>>>>> > > > > > > >>>>>>>> > > > > > >Thanks >>>>>>>> >>>>>>> > > > >Caching isn't guest visible. >>>>>> >>>>> > > >Seems not, if one fault mapping were cached by IOTLB. Guest can >>>>> notice this >>>>> > > >behavior. >>>>> >>>> > >Sorry, I don't get what you are saying. >>>> > > >>>> >>>>> > > > >Spec just says you*can* cache, >>>>>> > > > >not that you must. >>>>>> > > > > >>>>>> >>>>> > > >Yes, but what did in this patch is "don't". What I suggest is >>>>> just a "can", >>>>> > > >since anyway the IOTLB entries were limited and could be replaced >>>>> by other. >>>>> > > > >>>>> > > >Thanks >>>>> >>>> > >Have trouble understanding this. Can you given an example of >>>> > >a guest visible difference? >>>> >>> > >>> >I guess this may do the detection: >>> > >>> >1) map iova A to be non-present. >>> >2) invalidate iova A >>> >3) map iova A to addr B >>> >4) access iova A >>> > >>> >A correct implemented CM may meet fault in step 4, but with this patch, >>> we >>> >never. >>> >> I think that IOTLB is free to invalidate entries at any point, >> so the fault is not guaranteed on bare metal. >> > > Yes, that's the interesting point. The fault is not guaranteed but > conditional. And we have similar issue for IEC. > > So in conclusion (since I can't find a hardware IOMMU that have CM set): > > The spec says that there is no hardware IOMMU with CM bit set. This bit exists only to enable efficient emulation of iommu. > 1) If we don't cache fault conditions, we are still in question that > whether it was spec compatible. > 2) If we do cache fault conditions, we are 100% sure it was spec > compatible. > Consider 2) is not complicated, we'd better do it I believe? > > "For implementations reporting Caching Mode (CM) as 1 in the Capability Register, above conditions may cause caching of the entry that resulted in the fault, and require explicit invalidation by software to invalidate such cached entries." ( http://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/vt-directed-io-spec.pdf,, section 6.2.2, page 82). The key word is "may", therefore I do not think that actually faulting the guest is necessary in this case. However, if you insist on this part I will add the relevant code... Thanks > Aviv.
On Tue, Nov 22, 2016 at 12:11:21PM +0800, Jason Wang wrote: > Yes, that's the interesting point. The fault is not guaranteed but > conditional. And we have similar issue for IEC. > > So in conclusion (since I can't find a hardware IOMMU that have CM set): > > 1) If we don't cache fault conditions, we are still in question that whether > it was spec compatible. > 2) If we do cache fault conditions, we are 100% sure it was spec compatible. > > Consider 2) is not complicated, we'd better do it I believe? > > Thanks IMO it's just a confusing jargon used by intel architects. CM is there exactly so you can shadow the tables, but they were trying hard to use wording that also makes sense to hardware/driver developers. Nothing to worry about.
On Tue, Nov 22, 2016 at 03:15:45PM +0200, Aviv B.D. wrote:
> However, if you insist on this part I will add the relevant code...
I wouldn't bother at this point. Getting to a point where
we can merge it with vfio support seems more important.
> From: Michael S. Tsirkin > Sent: Tuesday, November 22, 2016 11:10 PM > > On Tue, Nov 22, 2016 at 12:11:21PM +0800, Jason Wang wrote: > > Yes, that's the interesting point. The fault is not guaranteed but > > conditional. And we have similar issue for IEC. > > > > So in conclusion (since I can't find a hardware IOMMU that have CM set): > > > > 1) If we don't cache fault conditions, we are still in question that whether > > it was spec compatible. > > 2) If we do cache fault conditions, we are 100% sure it was spec compatible. > > > > Consider 2) is not complicated, we'd better do it I believe? > > > > Thanks > > IMO it's just a confusing jargon used by intel architects. > CM is there exactly so you can shadow the tables, but > they were trying hard to use wording that also makes > sense to hardware/driver developers. > > Nothing to worry about. > Agree. Software shouldn't make any assumption that fault entries will be cached when caching mode is enabled. It's worded just because some very old generation did cache non-present/faulting entries so IOMMU driver needs to be aware of such potential side effect. while for virtualization the only purpose is to ask guest side to do IOTLB invalidation for any PTE change. It's completely fine to do simple thing here for vIOMMU here. Thanks Kevin
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 1655a65..834887f 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -2017,6 +2017,7 @@ static Property vtd_properties[] = { DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim, ON_OFF_AUTO_AUTO), DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false), + DEFINE_PROP_BOOL("cache-mode", IntelIOMMUState, cache_mode_enabled, FALSE), DEFINE_PROP_END_OF_LIST(), }; @@ -2391,6 +2392,10 @@ static void vtd_init(IntelIOMMUState *s) assert(s->intr_eim != ON_OFF_AUTO_AUTO); } + if (s->cache_mode_enabled) { + s->cap |= VTD_CAP_CM; + } + vtd_reset_context_cache(s); vtd_reset_iotlb(s); diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h index 0829a50..35d9f3a 100644 --- a/hw/i386/intel_iommu_internal.h +++ b/hw/i386/intel_iommu_internal.h @@ -201,6 +201,7 @@ #define VTD_CAP_MAMV (VTD_MAMV << 48) #define VTD_CAP_PSI (1ULL << 39) #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) +#define VTD_CAP_CM (1ULL << 7) /* Supported Adjusted Guest Address Widths */ #define VTD_CAP_SAGAW_SHIFT 8 diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 1989c1e..42d293f 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -258,6 +258,8 @@ struct IntelIOMMUState { uint8_t womask[DMAR_REG_SIZE]; /* WO (write only - read returns 0) */ uint32_t version; + bool cache_mode_enabled; /* RO - is cap CM enabled? */ + dma_addr_t root; /* Current root table pointer */ bool root_extended; /* Type of root table (extended or not) */ bool dmar_enabled; /* Set if DMA remapping is enabled */