| 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 |
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 >
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
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 >
>-----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
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
>-----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
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 --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;