diff mbox series

[v2,03/17] intel_iommu: Add a placeholder variable for scalable modern mode

Message ID 20240805062727.2307552-4-zhenzhong.duan@intel.com (mailing list archive)
State New, archived
Headers show
Series intel_iommu: Enable stage-1 translation for emulated device | expand

Commit Message

Duan, Zhenzhong Aug. 5, 2024, 6:27 a.m. UTC
Add an new element scalable_mode in IntelIOMMUState to mark scalable
modern mode, this element will be exposed as an intel_iommu property
finally.

For now, it's only a placehholder and used for address width
compatibility check and block host device passthrough until nesting
is supported.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/i386/intel_iommu.h |  1 +
 hw/i386/intel_iommu.c         | 12 +++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

CLEMENT MATHIEU--DRIF Aug. 6, 2024, 6:35 a.m. UTC | #1
On 05/08/2024 08:27, Zhenzhong Duan wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> Add an new element scalable_mode in IntelIOMMUState to mark scalable
> modern mode, this element will be exposed as an intel_iommu property
> finally.
>
> For now, it's only a placehholder and used for address width
> compatibility check and block host device passthrough until nesting
> is supported.
>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/i386/intel_iommu.h |  1 +
>   hw/i386/intel_iommu.c         | 12 +++++++++---
>   2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 1eb05c29fc..788ed42477 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -262,6 +262,7 @@ struct IntelIOMMUState {
>
>       bool caching_mode;              /* RO - is cap CM enabled? */
>       bool scalable_mode;             /* RO - is Scalable Mode supported? */
> +    bool scalable_modern;           /* RO - is modern SM supported? */
>       bool snoop_control;             /* RO - is SNP filed supported? */
>
>       dma_addr_t root;                /* Current root table pointer */
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index e3465fc27d..c1382a5651 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3872,7 +3872,13 @@ static bool vtd_check_hiod(IntelIOMMUState *s, HostIOMMUDevice *hiod,
>           return false;
>       }
>
> -    return true;
> +    if (!s->scalable_modern) {
> +        /* All checks requested by VTD non-modern mode pass */
> +        return true;
> +    }
> +
> +    error_setg(errp, "host device is unsupported in scalable modern mode yet");
> +    return false;
>   }
>
>   static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
> @@ -4262,9 +4268,9 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>           }
>       }
>
> -    /* Currently only address widths supported are 39 and 48 bits */
>       if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
> -        (s->aw_bits != VTD_HOST_AW_48BIT)) {
> +        (s->aw_bits != VTD_HOST_AW_48BIT) &&
> +        !s->scalable_modern) {
Why does scalable_modern allow to use a value other than 39 or 48?
Is it safe?
>           error_setg(errp, "Supported values for aw-bits are: %d, %d",
>                      VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT);
>           return false;
> --
> 2.34.1
>
Duan, Zhenzhong Aug. 8, 2024, 12:31 p.m. UTC | #2
On 8/6/2024 2:35 PM, CLEMENT MATHIEU--DRIF wrote:
>
> On 05/08/2024 08:27, Zhenzhong Duan wrote:
>> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>>
>>
>> Add an new element scalable_mode in IntelIOMMUState to mark scalable
>> modern mode, this element will be exposed as an intel_iommu property
>> finally.
>>
>> For now, it's only a placehholder and used for address width
>> compatibility check and block host device passthrough until nesting
>> is supported.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>    include/hw/i386/intel_iommu.h |  1 +
>>    hw/i386/intel_iommu.c         | 12 +++++++++---
>>    2 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
>> index 1eb05c29fc..788ed42477 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -262,6 +262,7 @@ struct IntelIOMMUState {
>>
>>        bool caching_mode;              /* RO - is cap CM enabled? */
>>        bool scalable_mode;             /* RO - is Scalable Mode supported? */
>> +    bool scalable_modern;           /* RO - is modern SM supported? */
>>        bool snoop_control;             /* RO - is SNP filed supported? */
>>
>>        dma_addr_t root;                /* Current root table pointer */
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index e3465fc27d..c1382a5651 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -3872,7 +3872,13 @@ static bool vtd_check_hiod(IntelIOMMUState *s, HostIOMMUDevice *hiod,
>>            return false;
>>        }
>>
>> -    return true;
>> +    if (!s->scalable_modern) {
>> +        /* All checks requested by VTD non-modern mode pass */
>> +        return true;
>> +    }
>> +
>> +    error_setg(errp, "host device is unsupported in scalable modern mode yet");
>> +    return false;
>>    }
>>
>>    static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>> @@ -4262,9 +4268,9 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>>            }
>>        }
>>
>> -    /* Currently only address widths supported are 39 and 48 bits */
>>        if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>> -        (s->aw_bits != VTD_HOST_AW_48BIT)) {
>> +        (s->aw_bits != VTD_HOST_AW_48BIT) &&
>> +        !s->scalable_modern) {
> Why does scalable_modern allow to use a value other than 39 or 48?
> Is it safe?

The check for scalable_modern is in patch14:

if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {

error_setg(errp, "Supported values for aw-bits are: %d", VTD_HOST_AW_48BIT);

return false;

}

Let me know if you prefer to move it in this patch.

Thanks

Zhenzhong
CLEMENT MATHIEU--DRIF Aug. 8, 2024, 3:04 p.m. UTC | #3
On 08/08/2024 14:31, Duan, Zhenzhong wrote:
> Caution: External email. Do not open attachments or click links, 
> unless this email comes from a known sender and you know the content 
> is safe.
>
>
> On 8/6/2024 2:35 PM, CLEMENT MATHIEU--DRIF wrote:
>>
>> On 05/08/2024 08:27, Zhenzhong Duan wrote:
>>> Caution: External email. Do not open attachments or click links, 
>>> unless this email comes from a known sender and you know the content 
>>> is safe.
>>>
>>>
>>> Add an new element scalable_mode in IntelIOMMUState to mark scalable
>>> modern mode, this element will be exposed as an intel_iommu property
>>> finally.
>>>
>>> For now, it's only a placehholder and used for address width
>>> compatibility check and block host device passthrough until nesting
>>> is supported.
>>>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    include/hw/i386/intel_iommu.h |  1 +
>>>    hw/i386/intel_iommu.c         | 12 +++++++++---
>>>    2 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/hw/i386/intel_iommu.h 
>>> b/include/hw/i386/intel_iommu.h
>>> index 1eb05c29fc..788ed42477 100644
>>> --- a/include/hw/i386/intel_iommu.h
>>> +++ b/include/hw/i386/intel_iommu.h
>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState {
>>>
>>>        bool caching_mode;              /* RO - is cap CM enabled? */
>>>        bool scalable_mode;             /* RO - is Scalable Mode 
>>> supported? */
>>> +    bool scalable_modern;           /* RO - is modern SM supported? */
>>>        bool snoop_control;             /* RO - is SNP filed 
>>> supported? */
>>>
>>>        dma_addr_t root;                /* Current root table pointer */
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index e3465fc27d..c1382a5651 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -3872,7 +3872,13 @@ static bool vtd_check_hiod(IntelIOMMUState 
>>> *s, HostIOMMUDevice *hiod,
>>>            return false;
>>>        }
>>>
>>> -    return true;
>>> +    if (!s->scalable_modern) {
>>> +        /* All checks requested by VTD non-modern mode pass */
>>> +        return true;
>>> +    }
>>> +
>>> +    error_setg(errp, "host device is unsupported in scalable modern 
>>> mode yet");
>>> +    return false;
>>>    }
>>>
>>>    static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, 
>>> int devfn,
>>> @@ -4262,9 +4268,9 @@ static bool vtd_decide_config(IntelIOMMUState 
>>> *s, Error **errp)
>>>            }
>>>        }
>>>
>>> -    /* Currently only address widths supported are 39 and 48 bits */
>>>        if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>>> -        (s->aw_bits != VTD_HOST_AW_48BIT)) {
>>> +        (s->aw_bits != VTD_HOST_AW_48BIT) &&
>>> +        !s->scalable_modern) {
>> Why does scalable_modern allow to use a value other than 39 or 48?
>> Is it safe?
>
> The check for scalable_modern is in patch14:
>
> if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {
>
> error_setg(errp, "Supported values for aw-bits are: %d", 
> VTD_HOST_AW_48BIT);
>
> return false;
>
> }
>
> Let me know if you prefer to move it in this patch.
Yes, you are right, it would be better to move the check here.

But I think the first check should also fail even when scalable_modern 
is enabled because values other than 39 and 48 are not supported at all, 
whatever the mode.
Then, we should check if the value is valid for scalable_modern mode.

Thanks
 >cmd
>
> Thanks
>
> Zhenzhong
>
Duan, Zhenzhong Aug. 13, 2024, 2:20 a.m. UTC | #4
>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: Re: [PATCH v2 03/17] intel_iommu: Add a placeholder variable for
>scalable modern mode
>
>
>
>On 08/08/2024 14:31, Duan, Zhenzhong wrote:
>> Caution: External email. Do not open attachments or click links,
>> unless this email comes from a known sender and you know the content
>> is safe.
>>
>>
>> On 8/6/2024 2:35 PM, CLEMENT MATHIEU--DRIF wrote:
>>>
>>> On 05/08/2024 08:27, Zhenzhong Duan wrote:
>>>> Caution: External email. Do not open attachments or click links,
>>>> unless this email comes from a known sender and you know the content
>>>> is safe.
>>>>
>>>>
>>>> Add an new element scalable_mode in IntelIOMMUState to mark
>scalable
>>>> modern mode, this element will be exposed as an intel_iommu property
>>>> finally.
>>>>
>>>> For now, it's only a placehholder and used for address width
>>>> compatibility check and block host device passthrough until nesting
>>>> is supported.
>>>>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>    include/hw/i386/intel_iommu.h |  1 +
>>>>    hw/i386/intel_iommu.c         | 12 +++++++++---
>>>>    2 files changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/hw/i386/intel_iommu.h
>>>> b/include/hw/i386/intel_iommu.h
>>>> index 1eb05c29fc..788ed42477 100644
>>>> --- a/include/hw/i386/intel_iommu.h
>>>> +++ b/include/hw/i386/intel_iommu.h
>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState {
>>>>
>>>>        bool caching_mode;              /* RO - is cap CM enabled? */
>>>>        bool scalable_mode;             /* RO - is Scalable Mode
>>>> supported? */
>>>> +    bool scalable_modern;           /* RO - is modern SM supported? */
>>>>        bool snoop_control;             /* RO - is SNP filed
>>>> supported? */
>>>>
>>>>        dma_addr_t root;                /* Current root table pointer */
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index e3465fc27d..c1382a5651 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -3872,7 +3872,13 @@ static bool
>vtd_check_hiod(IntelIOMMUState
>>>> *s, HostIOMMUDevice *hiod,
>>>>            return false;
>>>>        }
>>>>
>>>> -    return true;
>>>> +    if (!s->scalable_modern) {
>>>> +        /* All checks requested by VTD non-modern mode pass */
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    error_setg(errp, "host device is unsupported in scalable modern
>>>> mode yet");
>>>> +    return false;
>>>>    }
>>>>
>>>>    static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque,
>>>> int devfn,
>>>> @@ -4262,9 +4268,9 @@ static bool
>vtd_decide_config(IntelIOMMUState
>>>> *s, Error **errp)
>>>>            }
>>>>        }
>>>>
>>>> -    /* Currently only address widths supported are 39 and 48 bits */
>>>>        if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>>>> -        (s->aw_bits != VTD_HOST_AW_48BIT)) {
>>>> +        (s->aw_bits != VTD_HOST_AW_48BIT) &&
>>>> +        !s->scalable_modern) {
>>> Why does scalable_modern allow to use a value other than 39 or 48?
>>> Is it safe?
>>
>> The check for scalable_modern is in patch14:
>>
>> if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {
>>
>> error_setg(errp, "Supported values for aw-bits are: %d",
>> VTD_HOST_AW_48BIT);
>>
>> return false;
>>
>> }
>>
>> Let me know if you prefer to move it in this patch.
>Yes, you are right, it would be better to move the check here.
>
>But I think the first check should also fail even when scalable_modern
>is enabled because values other than 39 and 48 are not supported at all,
>whatever the mode.
>Then, we should check if the value is valid for scalable_modern mode.

Right, I wrote that way with a possible plan to support VTD_HOST_AW_52BIT.
What about this:

    if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
        (s->aw_bits != VTD_HOST_AW_48BIT) &&
        !s->scalable_modern) {
        error_setg(errp, "Scalable legacy mode: supported values for aw-bits are: %d, %d",
                   VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT);
        return false;
    }

    if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {
        error_setg(errp, "Scalable modern mode: supported values for aw-bits is: %d",
                   VTD_HOST_AW_48BIT);
        return false;
    }

Thanks
Zhenzhong
CLEMENT MATHIEU--DRIF Aug. 13, 2024, 5:22 a.m. UTC | #5
On 13/08/2024 04:20, Duan, Zhenzhong wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>> Subject: Re: [PATCH v2 03/17] intel_iommu: Add a placeholder variable for
>> scalable modern mode
>>
>>
>>
>> On 08/08/2024 14:31, Duan, Zhenzhong wrote:
>>> Caution: External email. Do not open attachments or click links,
>>> unless this email comes from a known sender and you know the content
>>> is safe.
>>>
>>>
>>> On 8/6/2024 2:35 PM, CLEMENT MATHIEU--DRIF wrote:
>>>> On 05/08/2024 08:27, Zhenzhong Duan wrote:
>>>>> Caution: External email. Do not open attachments or click links,
>>>>> unless this email comes from a known sender and you know the content
>>>>> is safe.
>>>>>
>>>>>
>>>>> Add an new element scalable_mode in IntelIOMMUState to mark
>> scalable
>>>>> modern mode, this element will be exposed as an intel_iommu property
>>>>> finally.
>>>>>
>>>>> For now, it's only a placehholder and used for address width
>>>>> compatibility check and block host device passthrough until nesting
>>>>> is supported.
>>>>>
>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>> ---
>>>>>     include/hw/i386/intel_iommu.h |  1 +
>>>>>     hw/i386/intel_iommu.c         | 12 +++++++++---
>>>>>     2 files changed, 10 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/i386/intel_iommu.h
>>>>> b/include/hw/i386/intel_iommu.h
>>>>> index 1eb05c29fc..788ed42477 100644
>>>>> --- a/include/hw/i386/intel_iommu.h
>>>>> +++ b/include/hw/i386/intel_iommu.h
>>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState {
>>>>>
>>>>>         bool caching_mode;              /* RO - is cap CM enabled? */
>>>>>         bool scalable_mode;             /* RO - is Scalable Mode
>>>>> supported? */
>>>>> +    bool scalable_modern;           /* RO - is modern SM supported? */
>>>>>         bool snoop_control;             /* RO - is SNP filed
>>>>> supported? */
>>>>>
>>>>>         dma_addr_t root;                /* Current root table pointer */
>>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>>> index e3465fc27d..c1382a5651 100644
>>>>> --- a/hw/i386/intel_iommu.c
>>>>> +++ b/hw/i386/intel_iommu.c
>>>>> @@ -3872,7 +3872,13 @@ static bool
>> vtd_check_hiod(IntelIOMMUState
>>>>> *s, HostIOMMUDevice *hiod,
>>>>>             return false;
>>>>>         }
>>>>>
>>>>> -    return true;
>>>>> +    if (!s->scalable_modern) {
>>>>> +        /* All checks requested by VTD non-modern mode pass */
>>>>> +        return true;
>>>>> +    }
>>>>> +
>>>>> +    error_setg(errp, "host device is unsupported in scalable modern
>>>>> mode yet");
>>>>> +    return false;
>>>>>     }
>>>>>
>>>>>     static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque,
>>>>> int devfn,
>>>>> @@ -4262,9 +4268,9 @@ static bool
>> vtd_decide_config(IntelIOMMUState
>>>>> *s, Error **errp)
>>>>>             }
>>>>>         }
>>>>>
>>>>> -    /* Currently only address widths supported are 39 and 48 bits */
>>>>>         if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>>>>> -        (s->aw_bits != VTD_HOST_AW_48BIT)) {
>>>>> +        (s->aw_bits != VTD_HOST_AW_48BIT) &&
>>>>> +        !s->scalable_modern) {
>>>> Why does scalable_modern allow to use a value other than 39 or 48?
>>>> Is it safe?
>>> The check for scalable_modern is in patch14:
>>>
>>> if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {
>>>
>>> error_setg(errp, "Supported values for aw-bits are: %d",
>>> VTD_HOST_AW_48BIT);
>>>
>>> return false;
>>>
>>> }
>>>
>>> Let me know if you prefer to move it in this patch.
>> Yes, you are right, it would be better to move the check here.
>>
>> But I think the first check should also fail even when scalable_modern
>> is enabled because values other than 39 and 48 are not supported at all,
>> whatever the mode.
>> Then, we should check if the value is valid for scalable_modern mode.
> Right, I wrote that way with a possible plan to support VTD_HOST_AW_52BIT.
52 or 57?
> What about this:
>
This condition traps (non-scalable) legacy mode as well. I think we 
should change the error message to make it clear
Something like this: "Legacy and non-modern scalable modes: supported 
values for aw-bit are ..."
Or we could make the error message conditional.
>      if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>          (s->aw_bits != VTD_HOST_AW_48BIT) &&
>          !s->scalable_modern) {
>          error_setg(errp, "Scalable legacy mode: supported values for aw-bits are: %d, %d",
>                     VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT);
>          return false;
>      }
>
>      if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {
>          error_setg(errp, "Scalable modern mode: supported values for aw-bits is: %d",
>                     VTD_HOST_AW_48BIT);
>          return false;
>      }


>
> Thanks
> Zhenzhong
Duan, Zhenzhong Aug. 13, 2024, 6:26 a.m. UTC | #6
>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: Re: [PATCH v2 03/17] intel_iommu: Add a placeholder variable for
>scalable modern mode
>
>
>
>On 13/08/2024 04:20, Duan, Zhenzhong wrote:
>> Caution: External email. Do not open attachments or click links, unless this
>email comes from a known sender and you know the content is safe.
>>
>>
>>> -----Original Message-----
>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>>> Subject: Re: [PATCH v2 03/17] intel_iommu: Add a placeholder variable
>for
>>> scalable modern mode
>>>
>>>
>>>
>>> On 08/08/2024 14:31, Duan, Zhenzhong wrote:
>>>> Caution: External email. Do not open attachments or click links,
>>>> unless this email comes from a known sender and you know the content
>>>> is safe.
>>>>
>>>>
>>>> On 8/6/2024 2:35 PM, CLEMENT MATHIEU--DRIF wrote:
>>>>> On 05/08/2024 08:27, Zhenzhong Duan wrote:
>>>>>> Caution: External email. Do not open attachments or click links,
>>>>>> unless this email comes from a known sender and you know the
>content
>>>>>> is safe.
>>>>>>
>>>>>>
>>>>>> Add an new element scalable_mode in IntelIOMMUState to mark
>>> scalable
>>>>>> modern mode, this element will be exposed as an intel_iommu
>property
>>>>>> finally.
>>>>>>
>>>>>> For now, it's only a placehholder and used for address width
>>>>>> compatibility check and block host device passthrough until nesting
>>>>>> is supported.
>>>>>>
>>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>>> ---
>>>>>>     include/hw/i386/intel_iommu.h |  1 +
>>>>>>     hw/i386/intel_iommu.c         | 12 +++++++++---
>>>>>>     2 files changed, 10 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/i386/intel_iommu.h
>>>>>> b/include/hw/i386/intel_iommu.h
>>>>>> index 1eb05c29fc..788ed42477 100644
>>>>>> --- a/include/hw/i386/intel_iommu.h
>>>>>> +++ b/include/hw/i386/intel_iommu.h
>>>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState {
>>>>>>
>>>>>>         bool caching_mode;              /* RO - is cap CM enabled? */
>>>>>>         bool scalable_mode;             /* RO - is Scalable Mode
>>>>>> supported? */
>>>>>> +    bool scalable_modern;           /* RO - is modern SM supported? */
>>>>>>         bool snoop_control;             /* RO - is SNP filed
>>>>>> supported? */
>>>>>>
>>>>>>         dma_addr_t root;                /* Current root table pointer */
>>>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>>>> index e3465fc27d..c1382a5651 100644
>>>>>> --- a/hw/i386/intel_iommu.c
>>>>>> +++ b/hw/i386/intel_iommu.c
>>>>>> @@ -3872,7 +3872,13 @@ static bool
>>> vtd_check_hiod(IntelIOMMUState
>>>>>> *s, HostIOMMUDevice *hiod,
>>>>>>             return false;
>>>>>>         }
>>>>>>
>>>>>> -    return true;
>>>>>> +    if (!s->scalable_modern) {
>>>>>> +        /* All checks requested by VTD non-modern mode pass */
>>>>>> +        return true;
>>>>>> +    }
>>>>>> +
>>>>>> +    error_setg(errp, "host device is unsupported in scalable modern
>>>>>> mode yet");
>>>>>> +    return false;
>>>>>>     }
>>>>>>
>>>>>>     static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque,
>>>>>> int devfn,
>>>>>> @@ -4262,9 +4268,9 @@ static bool
>>> vtd_decide_config(IntelIOMMUState
>>>>>> *s, Error **errp)
>>>>>>             }
>>>>>>         }
>>>>>>
>>>>>> -    /* Currently only address widths supported are 39 and 48 bits */
>>>>>>         if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>>>>>> -        (s->aw_bits != VTD_HOST_AW_48BIT)) {
>>>>>> +        (s->aw_bits != VTD_HOST_AW_48BIT) &&
>>>>>> +        !s->scalable_modern) {
>>>>> Why does scalable_modern allow to use a value other than 39 or 48?
>>>>> Is it safe?
>>>> The check for scalable_modern is in patch14:
>>>>
>>>> if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {
>>>>
>>>> error_setg(errp, "Supported values for aw-bits are: %d",
>>>> VTD_HOST_AW_48BIT);
>>>>
>>>> return false;
>>>>
>>>> }
>>>>
>>>> Let me know if you prefer to move it in this patch.
>>> Yes, you are right, it would be better to move the check here.
>>>
>>> But I think the first check should also fail even when scalable_modern
>>> is enabled because values other than 39 and 48 are not supported at all,
>>> whatever the mode.
>>> Then, we should check if the value is valid for scalable_modern mode.
>> Right, I wrote that way with a possible plan to support
>VTD_HOST_AW_52BIT.
>52 or 57?

Sorry, I mean 57.

>> What about this:
>>
>This condition traps (non-scalable) legacy mode as well. I think we
>should change the error message to make it clear
>Something like this: "Legacy and non-modern scalable modes: supported
>values for aw-bit are ..."
>Or we could make the error message conditional.

Yes, I'd like to be conditional, like:

    if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
        (s->aw_bits != VTD_HOST_AW_48BIT) &&
        !s->scalable_modern) {
        error_setg(errp, "%s mode: supported values for aw-bits are: %d, %d",
                   s->scalable_mode ? "Scalable legacy" : "Legacy",
                   VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT);
        return false;
    }

>>      if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>>          (s->aw_bits != VTD_HOST_AW_48BIT) &&
>>          !s->scalable_modern) {
>>          error_setg(errp, "Scalable legacy mode: supported values for aw-bits
>are: %d, %d",
>>                     VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT);
>>          return false;
>>      }
>>
>>      if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {
>>          error_setg(errp, "Scalable modern mode: supported values for aw-
>bits is: %d",
>>                     VTD_HOST_AW_48BIT);
>>          return false;
>>      }
>
>
>>
>> Thanks
>> Zhenzhong
CLEMENT MATHIEU--DRIF Aug. 13, 2024, 6:58 a.m. UTC | #7
On 13/08/2024 08:26, Duan, Zhenzhong wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>> Subject: Re: [PATCH v2 03/17] intel_iommu: Add a placeholder variable for
>> scalable modern mode
>>
>>
>>
>> On 13/08/2024 04:20, Duan, Zhenzhong wrote:
>>> Caution: External email. Do not open attachments or click links, unless this
>> email comes from a known sender and you know the content is safe.
>>>
>>>> -----Original Message-----
>>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>>>> Subject: Re: [PATCH v2 03/17] intel_iommu: Add a placeholder variable
>> for
>>>> scalable modern mode
>>>>
>>>>
>>>>
>>>> On 08/08/2024 14:31, Duan, Zhenzhong wrote:
>>>>> Caution: External email. Do not open attachments or click links,
>>>>> unless this email comes from a known sender and you know the content
>>>>> is safe.
>>>>>
>>>>>
>>>>> On 8/6/2024 2:35 PM, CLEMENT MATHIEU--DRIF wrote:
>>>>>> On 05/08/2024 08:27, Zhenzhong Duan wrote:
>>>>>>> Caution: External email. Do not open attachments or click links,
>>>>>>> unless this email comes from a known sender and you know the
>> content
>>>>>>> is safe.
>>>>>>>
>>>>>>>
>>>>>>> Add an new element scalable_mode in IntelIOMMUState to mark
>>>> scalable
>>>>>>> modern mode, this element will be exposed as an intel_iommu
>> property
>>>>>>> finally.
>>>>>>>
>>>>>>> For now, it's only a placehholder and used for address width
>>>>>>> compatibility check and block host device passthrough until nesting
>>>>>>> is supported.
>>>>>>>
>>>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>>>> ---
>>>>>>>      include/hw/i386/intel_iommu.h |  1 +
>>>>>>>      hw/i386/intel_iommu.c         | 12 +++++++++---
>>>>>>>      2 files changed, 10 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/hw/i386/intel_iommu.h
>>>>>>> b/include/hw/i386/intel_iommu.h
>>>>>>> index 1eb05c29fc..788ed42477 100644
>>>>>>> --- a/include/hw/i386/intel_iommu.h
>>>>>>> +++ b/include/hw/i386/intel_iommu.h
>>>>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState {
>>>>>>>
>>>>>>>          bool caching_mode;              /* RO - is cap CM enabled? */
>>>>>>>          bool scalable_mode;             /* RO - is Scalable Mode
>>>>>>> supported? */
>>>>>>> +    bool scalable_modern;           /* RO - is modern SM supported? */
>>>>>>>          bool snoop_control;             /* RO - is SNP filed
>>>>>>> supported? */
>>>>>>>
>>>>>>>          dma_addr_t root;                /* Current root table pointer */
>>>>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>>>>> index e3465fc27d..c1382a5651 100644
>>>>>>> --- a/hw/i386/intel_iommu.c
>>>>>>> +++ b/hw/i386/intel_iommu.c
>>>>>>> @@ -3872,7 +3872,13 @@ static bool
>>>> vtd_check_hiod(IntelIOMMUState
>>>>>>> *s, HostIOMMUDevice *hiod,
>>>>>>>              return false;
>>>>>>>          }
>>>>>>>
>>>>>>> -    return true;
>>>>>>> +    if (!s->scalable_modern) {
>>>>>>> +        /* All checks requested by VTD non-modern mode pass */
>>>>>>> +        return true;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    error_setg(errp, "host device is unsupported in scalable modern
>>>>>>> mode yet");
>>>>>>> +    return false;
>>>>>>>      }
>>>>>>>
>>>>>>>      static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque,
>>>>>>> int devfn,
>>>>>>> @@ -4262,9 +4268,9 @@ static bool
>>>> vtd_decide_config(IntelIOMMUState
>>>>>>> *s, Error **errp)
>>>>>>>              }
>>>>>>>          }
>>>>>>>
>>>>>>> -    /* Currently only address widths supported are 39 and 48 bits */
>>>>>>>          if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>>>>>>> -        (s->aw_bits != VTD_HOST_AW_48BIT)) {
>>>>>>> +        (s->aw_bits != VTD_HOST_AW_48BIT) &&
>>>>>>> +        !s->scalable_modern) {
>>>>>> Why does scalable_modern allow to use a value other than 39 or 48?
>>>>>> Is it safe?
>>>>> The check for scalable_modern is in patch14:
>>>>>
>>>>> if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {
>>>>>
>>>>> error_setg(errp, "Supported values for aw-bits are: %d",
>>>>> VTD_HOST_AW_48BIT);
>>>>>
>>>>> return false;
>>>>>
>>>>> }
>>>>>
>>>>> Let me know if you prefer to move it in this patch.
>>>> Yes, you are right, it would be better to move the check here.
>>>>
>>>> But I think the first check should also fail even when scalable_modern
>>>> is enabled because values other than 39 and 48 are not supported at all,
>>>> whatever the mode.
>>>> Then, we should check if the value is valid for scalable_modern mode.
>>> Right, I wrote that way with a possible plan to support
>> VTD_HOST_AW_52BIT.
>> 52 or 57?
> Sorry, I mean 57.
>
>>> What about this:
>>>
>> This condition traps (non-scalable) legacy mode as well. I think we
>> should change the error message to make it clear
>> Something like this: "Legacy and non-modern scalable modes: supported
>> values for aw-bit are ..."
>> Or we could make the error message conditional.
> Yes, I'd like to be conditional, like:
>
>      if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>          (s->aw_bits != VTD_HOST_AW_48BIT) &&
>          !s->scalable_modern) {
>          error_setg(errp, "%s mode: supported values for aw-bits are: %d, %d",
>                     s->scalable_mode ? "Scalable legacy" : "Legacy",
>                     VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT);
>          return false;
>      }
Fine,
lgtm

 >cmd
>>>       if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>>>           (s->aw_bits != VTD_HOST_AW_48BIT) &&
>>>           !s->scalable_modern) {
>>>           error_setg(errp, "Scalable legacy mode: supported values for aw-bits
>> are: %d, %d",
>>>                      VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT);
>>>           return false;
>>>       }
>>>
>>>       if ((s->aw_bits != VTD_HOST_AW_48BIT) && s->scalable_modern) {
>>>           error_setg(errp, "Scalable modern mode: supported values for aw-
>> bits is: %d",
>>>                      VTD_HOST_AW_48BIT);
>>>           return false;
>>>       }
>>
>>> Thanks
>>> Zhenzhong
diff mbox series

Patch

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 1eb05c29fc..788ed42477 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -262,6 +262,7 @@  struct IntelIOMMUState {
 
     bool caching_mode;              /* RO - is cap CM enabled? */
     bool scalable_mode;             /* RO - is Scalable Mode supported? */
+    bool scalable_modern;           /* RO - is modern SM supported? */
     bool snoop_control;             /* RO - is SNP filed supported? */
 
     dma_addr_t root;                /* Current root table pointer */
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index e3465fc27d..c1382a5651 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3872,7 +3872,13 @@  static bool vtd_check_hiod(IntelIOMMUState *s, HostIOMMUDevice *hiod,
         return false;
     }
 
-    return true;
+    if (!s->scalable_modern) {
+        /* All checks requested by VTD non-modern mode pass */
+        return true;
+    }
+
+    error_setg(errp, "host device is unsupported in scalable modern mode yet");
+    return false;
 }
 
 static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
@@ -4262,9 +4268,9 @@  static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
         }
     }
 
-    /* Currently only address widths supported are 39 and 48 bits */
     if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
-        (s->aw_bits != VTD_HOST_AW_48BIT)) {
+        (s->aw_bits != VTD_HOST_AW_48BIT) &&
+        !s->scalable_modern) {
         error_setg(errp, "Supported values for aw-bits are: %d, %d",
                    VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT);
         return false;