[v6,1/3] IOMMU: add option to enable VTD_CAP_CM to vIOMMU capility exposoed to guest

Commit Message

Aviv B.D. Nov. 8, 2016, 11:04 a.m. UTC

From: "Aviv Ben-David" <[e-mail]>

This capability asks the guest to invalidate cache before each map operation.
We can use this invalidation to trap map operations in the hypervisor.

Signed-off-by: Aviv Ben-David <[e-mail]>
---
 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(+)

Comments

Jason Wang Nov. 9, 2016, 7:28 a.m. UTC | #1

On 2016年11月08日 19:04, Aviv B.D wrote:
> From: "Aviv Ben-David" <[e-mail]>
>
> 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 <[e-mail]>
> ---
>   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 */

Michael S. Tsirkin Nov. 9, 2016, 10 p.m. UTC | #2

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" <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >   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 */

Jason Wang Nov. 11, 2016, 2:32 a.m. UTC | #3

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"<[e-mail]>
>>> > >
>>> > >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

Michael S. Tsirkin Nov. 11, 2016, 3:39 a.m. UTC | #4

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"<[e-mail]>
> > > > > >
> > > > > >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?

Jason Wang Nov. 11, 2016, 4:15 a.m. UTC | #5

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"<[e-mail]>
>>>>>>>
>>>>>>> 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.

Aviv B.D. Nov. 21, 2016, 12:41 p.m. UTC | #6

On Fri, Nov 11, 2016 at 6:15 AM, Jason Wang <[e-mail]> 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"<[e-mail]>
>>>>>>>>
>>>>>>>> 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.

Michael S. Tsirkin Nov. 21, 2016, 1:35 p.m. UTC | #7

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"<[e-mail]>
> > > > > > > > 
> > > > > > > > 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.

Jason Wang Nov. 22, 2016, 3:59 a.m. UTC | #8

On 2016年11月21日 20:41, Aviv B.D. wrote:
>
>
> On Fri, Nov 11, 2016 at 6:15 AM, Jason Wang <[e-mail] 
> <mailto:[e-mail]>> 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"<[e-mail]
>                                 <mailto:[e-mail]>>
>
>                                 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.

Jason Wang Nov. 22, 2016, 4:11 a.m. UTC | #9

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"<[e-mail]>
>>>>>>>>> > > > > > > > >
>>>>>>>>> > > > > > > > >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

Aviv B.D. Nov. 22, 2016, 1:15 p.m. UTC | #10

On Tue, Nov 22, 2016 at 6:11 AM, Jason Wang <[e-mail]> 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"<[e-mail]>
>>>>>>>>>> > > > > > > > >
>>>>>>>>>> > > > > > > > >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.

Michael S. Tsirkin Nov. 22, 2016, 3:09 p.m. UTC | #11

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.

Michael S. Tsirkin Nov. 22, 2016, 3:13 p.m. UTC | #12

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.

Tian, Kevin Dec. 1, 2016, 3:02 a.m. UTC | #13

> 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

Patch

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 */