Message ID | 20240702055221.1337035-5-clement.mathieu--drif@eviden.com |
---|---|
State | New |
Headers | show |
Series | ATS support for VT-d | expand |
On 2024/7/2 13:52, CLEMENT MATHIEU--DRIF wrote: > From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> > > Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> > Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/i386/intel_iommu.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 98996ededc..71cebe2fd3 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -3500,6 +3500,11 @@ static bool vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) > } else if (inv_desc->lo & VTD_INV_DESC_WAIT_IF) { > /* Interrupt flag */ > vtd_generate_completion_event(s); > + } else if (inv_desc->lo & VTD_INV_DESC_WAIT_FN) { > + /* > + * SW = 0, IF = 0, FN = 1 > + * Nothing to do as we process the events sequentially > + */ This code looks a bit weird. SW field does not co-exist with IF. But either SW or IF can co-exist with FN flag. Is it? Have you already seen a wait descriptor that only has FN flag set but no SW nor IF flag? > } else { > error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64 > " (unknown type)", __func__, inv_desc->hi,
On 02/07/2024 15:33, Yi Liu 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 2024/7/2 13:52, CLEMENT MATHIEU--DRIF wrote: >> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> >> >> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> >> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> hw/i386/intel_iommu.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 98996ededc..71cebe2fd3 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -3500,6 +3500,11 @@ static bool >> vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) >> } else if (inv_desc->lo & VTD_INV_DESC_WAIT_IF) { >> /* Interrupt flag */ >> vtd_generate_completion_event(s); >> + } else if (inv_desc->lo & VTD_INV_DESC_WAIT_FN) { >> + /* >> + * SW = 0, IF = 0, FN = 1 >> + * Nothing to do as we process the events sequentially >> + */ > > This code looks a bit weird. SW field does not co-exist with IF. But > either > SW or IF can co-exist with FN flag. Is it? Have you already seen a wait > descriptor that only has FN flag set but no SW nor IF flag? Yes, my test suite triggers that condition > >> } else { >> error_report_once("%s: invalid wait desc: hi=%"PRIx64", >> lo=%"PRIx64 >> " (unknown type)", __func__, inv_desc->hi, > > -- > Regards, > Yi Liu
On 02/07/2024 17:29, CLEMENT MATHIEU--DRIF wrote: > On 02/07/2024 15:33, Yi Liu 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 2024/7/2 13:52, CLEMENT MATHIEU--DRIF wrote: >>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> >>> >>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> >>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> hw/i386/intel_iommu.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>> index 98996ededc..71cebe2fd3 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -3500,6 +3500,11 @@ static bool >>> vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) >>> } else if (inv_desc->lo & VTD_INV_DESC_WAIT_IF) { >>> /* Interrupt flag */ >>> vtd_generate_completion_event(s); >>> + } else if (inv_desc->lo & VTD_INV_DESC_WAIT_FN) { >>> + /* >>> + * SW = 0, IF = 0, FN = 1 >>> + * Nothing to do as we process the events sequentially >>> + */ >> This code looks a bit weird. SW field does not co-exist with IF. But >> either >> SW or IF can co-exist with FN flag. Is it? Have you already seen a wait >> descriptor that only has FN flag set but no SW nor IF flag? > Yes, my test suite triggers that condition I think it comes from the kernel function intel_drain_pasid_prq (https://elixir.bootlin.com/linux/latest/source/drivers/iommu/intel/svm.c#L467) >>> } else { >>> error_report_once("%s: invalid wait desc: hi=%"PRIx64", >>> lo=%"PRIx64 >>> " (unknown type)", __func__, inv_desc->hi, >> -- >> Regards, >> Yi Liu
On 2024/7/2 23:29, CLEMENT MATHIEU--DRIF wrote: > > On 02/07/2024 15:33, Yi Liu 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 2024/7/2 13:52, CLEMENT MATHIEU--DRIF wrote: >>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> >>> >>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> >>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> hw/i386/intel_iommu.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>> index 98996ededc..71cebe2fd3 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -3500,6 +3500,11 @@ static bool >>> vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) >>> } else if (inv_desc->lo & VTD_INV_DESC_WAIT_IF) { >>> /* Interrupt flag */ >>> vtd_generate_completion_event(s); >>> + } else if (inv_desc->lo & VTD_INV_DESC_WAIT_FN) { >>> + /* >>> + * SW = 0, IF = 0, FN = 1 >>> + * Nothing to do as we process the events sequentially >>> + */ >> >> This code looks a bit weird. SW field does not co-exist with IF. But >> either >> SW or IF can co-exist with FN flag. Is it? Have you already seen a wait >> descriptor that only has FN flag set but no SW nor IF flag? > Yes, my test suite triggers that condition I see. Spec indeed has such usage. Please add a comment for it. Since it does not need a response, so QEMU can just bypass it. Also please adjust the subject a bit. It's misleading. Perhaps "intel_iommu: Bypass barrier wait descriptor" Spec CH 7.10 a. Submit Invalidation Wait Descriptor (inv_wait_dsc) with Fence flag (FN=1) Set to Invalidation Queue. This ensures that all requests submitted to the Invalidation Queue ahead of this wait descriptor are processed and completed by remapping hardware before processing requests after the Invalidation Wait Descriptor. It is not required to specify SW flag (or IF flag) in this descriptor or for software to wait on its completion, as its function is to only act as a barrier. >> >>> } else { >>> error_report_once("%s: invalid wait desc: hi=%"PRIx64", >>> lo=%"PRIx64 >>> " (unknown type)", __func__, inv_desc->hi, >> >> -- >> Regards, >> Yi Liu
On 03/07/2024 09:29, Yi Liu wrote: > On 2024/7/2 23:29, CLEMENT MATHIEU--DRIF wrote: >> >> On 02/07/2024 15:33, Yi Liu 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 2024/7/2 13:52, CLEMENT MATHIEU--DRIF wrote: >>>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> >>>> >>>> Signed-off-by: Clément Mathieu--Drif >>>> <clement.mathieu--drif@eviden.com> >>>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> hw/i386/intel_iommu.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>> index 98996ededc..71cebe2fd3 100644 >>>> --- a/hw/i386/intel_iommu.c >>>> +++ b/hw/i386/intel_iommu.c >>>> @@ -3500,6 +3500,11 @@ static bool >>>> vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) >>>> } else if (inv_desc->lo & VTD_INV_DESC_WAIT_IF) { >>>> /* Interrupt flag */ >>>> vtd_generate_completion_event(s); >>>> + } else if (inv_desc->lo & VTD_INV_DESC_WAIT_FN) { >>>> + /* >>>> + * SW = 0, IF = 0, FN = 1 >>>> + * Nothing to do as we process the events sequentially >>>> + */ >>> >>> This code looks a bit weird. SW field does not co-exist with IF. But >>> either >>> SW or IF can co-exist with FN flag. Is it? Have you already seen a wait >>> descriptor that only has FN flag set but no SW nor IF flag? >> Yes, my test suite triggers that condition > > I see. Spec indeed has such usage. Please add a comment for it. > Since it does not need a response, so QEMU can just bypass it. Also > please adjust the subject a bit. It's misleading. Perhaps > > "intel_iommu: Bypass barrier wait descriptor" Fine, will do > > Spec CH 7.10 > a. Submit Invalidation Wait Descriptor (inv_wait_dsc) with Fence flag > (FN=1) Set to Invalidation > Queue. This ensures that all requests submitted to the Invalidation > Queue ahead of this wait > descriptor are processed and completed by remapping hardware before > processing requests > after the Invalidation Wait Descriptor. It is not required to specify > SW flag (or IF flag) in this > descriptor or for software to wait on its completion, as its function > is to only act as a barrier. > >>> >>>> } else { >>>> error_report_once("%s: invalid wait desc: hi=%"PRIx64", >>>> lo=%"PRIx64 >>>> " (unknown type)", __func__, >>>> inv_desc->hi, >>> >>> -- >>> Regards, >>> Yi Liu >
On 03/07/2024 09:29, Yi Liu 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 2024/7/2 23:29, CLEMENT MATHIEU--DRIF wrote: >> >> On 02/07/2024 15:33, Yi Liu 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 2024/7/2 13:52, CLEMENT MATHIEU--DRIF wrote: >>>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com> >>>> >>>> Signed-off-by: Clément Mathieu--Drif >>>> <clement.mathieu--drif@eviden.com> >>>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> hw/i386/intel_iommu.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>> index 98996ededc..71cebe2fd3 100644 >>>> --- a/hw/i386/intel_iommu.c >>>> +++ b/hw/i386/intel_iommu.c >>>> @@ -3500,6 +3500,11 @@ static bool >>>> vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) >>>> } else if (inv_desc->lo & VTD_INV_DESC_WAIT_IF) { >>>> /* Interrupt flag */ >>>> vtd_generate_completion_event(s); >>>> + } else if (inv_desc->lo & VTD_INV_DESC_WAIT_FN) { >>>> + /* >>>> + * SW = 0, IF = 0, FN = 1 >>>> + * Nothing to do as we process the events sequentially >>>> + */ >>> >>> This code looks a bit weird. SW field does not co-exist with IF. But >>> either >>> SW or IF can co-exist with FN flag. Is it? Have you already seen a wait >>> descriptor that only has FN flag set but no SW nor IF flag? >> Yes, my test suite triggers that condition > > I see. Spec indeed has such usage. Please add a comment for it. > Since it does not need a response, so QEMU can just bypass it. Also > please adjust the subject a bit. It's misleading. Perhaps > > "intel_iommu: Bypass barrier wait descriptor" good idea, will do > > Spec CH 7.10 > a. Submit Invalidation Wait Descriptor (inv_wait_dsc) with Fence flag > (FN=1) Set to Invalidation > Queue. This ensures that all requests submitted to the Invalidation Queue > ahead of this wait > descriptor are processed and completed by remapping hardware before > processing requests > after the Invalidation Wait Descriptor. It is not required to specify SW > flag (or IF flag) in this > descriptor or for software to wait on its completion, as its function > is to > only act as a barrier. > >>> >>>> } else { >>>> error_report_once("%s: invalid wait desc: hi=%"PRIx64", >>>> lo=%"PRIx64 >>>> " (unknown type)", __func__, >>>> inv_desc->hi, >>> >>> -- >>> Regards, >>> Yi Liu > > -- > Regards, > Yi Liu
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 98996ededc..71cebe2fd3 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3500,6 +3500,11 @@ static bool vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) } else if (inv_desc->lo & VTD_INV_DESC_WAIT_IF) { /* Interrupt flag */ vtd_generate_completion_event(s); + } else if (inv_desc->lo & VTD_INV_DESC_WAIT_FN) { + /* + * SW = 0, IF = 0, FN = 1 + * Nothing to do as we process the events sequentially + */ } else { error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64 " (unknown type)", __func__, inv_desc->hi,