| Message ID | c70b250d-2ec4-4254-89cf-d3241dac0d35@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VT-d: SATC handling; ATS: tidying | expand |
On Thu, Feb 15, 2024 at 11:14:31AM +0100, Jan Beulich wrote: > This is a prereq to us, in particular, respecting the "ATC required" > flag. > > Note that ACPI_SATC_ATC_REQUIRED has its #define put in dmar.h, as we > try to keep actbl*.h in sync what Linux (who in turn inherit from ACPI > CA) has. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Lovely: On the SPR system with the SATC I tried passing "ats" (the > "required" flag is clear there), just to then hit "IOMMU#4: QI dev wait > descriptor taking too long" while setting up Dom0. The 2nd message there > doesn't ever appear, so the request never completes. Not sure whether > that's us doing something wrong or the hardware acting up. In the former > case I'd generally expect an IOMMU fault to be raised, though. FTR same > on 4.18 with just "VT-d: correct ATS checking for root complex > integrated devices" backported there. Great, so we likely have a bug in our ATS implementation? > > Should we check scope entries for appropriate types? (If so, then also > for e.g. ATSR.) > --- > v2: Move error case freeing to acpi_parse_one_satc(). Introduce #define > for the flag bit. Style. > > --- a/xen/drivers/passthrough/vtd/dmar.c > +++ b/xen/drivers/passthrough/vtd/dmar.c > @@ -47,6 +47,7 @@ LIST_HEAD_READ_MOSTLY(acpi_drhd_units); > LIST_HEAD_READ_MOSTLY(acpi_rmrr_units); > static LIST_HEAD_READ_MOSTLY(acpi_atsr_units); > static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units); > +static LIST_HEAD_READ_MOSTLY(acpi_satc_units); We could even make this one RO after init. > > static struct acpi_table_header *__read_mostly dmar_table; > static int __read_mostly dmar_flags; > @@ -750,6 +751,93 @@ acpi_parse_one_rhsa(struct acpi_dmar_hea > return ret; > } > > +static int __init register_one_satc(struct acpi_satc_unit *satcu) > +{ > + bool ignore = false; > + unsigned int i = 0; > + int ret = 0; > + > + /* Skip checking if segment is not accessible yet. */ > + if ( !pci_known_segment(satcu->segment) ) > + i = UINT_MAX; > + > + for ( ; i < satcu->scope.devices_cnt; i++ ) > + { > + uint8_t b = PCI_BUS(satcu->scope.devices[i]); > + uint8_t d = PCI_SLOT(satcu->scope.devices[i]); > + uint8_t f = PCI_FUNC(satcu->scope.devices[i]); > + > + if ( !pci_device_detect(satcu->segment, b, d, f) ) > + { > + dprintk(XENLOG_WARNING VTDPREFIX, > + " Non-existent device (%pp) is reported in SATC scope!\n", > + &PCI_SBDF(satcu->segment, b, d, f)); > + ignore = true; > + } > + else > + { > + ignore = false; > + break; > + } > + } > + > + if ( ignore ) > + { > + dprintk(XENLOG_WARNING VTDPREFIX, > + " Ignore SATC for seg %04x as no device under its scope is PCI discoverable\n", > + satcu->segment); Re the error messages: won't it be better to print them using plain printk and gate on iommu_verbose being enabled if anything? It does seem a bit odd that such messages won't be printed when iommu={debug,verbose} is enabled on the command line. > + return 1; > + } > + > + if ( iommu_verbose ) > + printk(VTDPREFIX " ATC required: %d\n", satcu->atc_required); > + > + list_add(&satcu->list, &acpi_satc_units); > + > + return ret; > +} > + > +static int __init > +acpi_parse_one_satc(const struct acpi_dmar_header *header) > +{ > + const struct acpi_dmar_satc *satc = > + container_of(header, const struct acpi_dmar_satc, header); > + struct acpi_satc_unit *satcu; > + const void *dev_scope_start, *dev_scope_end; > + int ret = acpi_dmar_check_length(header, sizeof(*satc)); > + > + if ( ret ) > + return ret; > + > + satcu = xzalloc(struct acpi_satc_unit); > + if ( !satcu ) > + return -ENOMEM; > + > + satcu->segment = satc->segment; > + satcu->atc_required = satc->flags & ACPI_SATC_ATC_REQUIRED; > + > + dev_scope_start = (const void *)(satc + 1); > + dev_scope_end = (const void *)satc + header->length; Isn't it enough to just cast to void * and inherit the const from the left side variable declaration? You could even initialize dev_scope_{start,end} at definition. Thanks, Roger.
On 06.05.2024 12:29, Roger Pau Monné wrote: > On Thu, Feb 15, 2024 at 11:14:31AM +0100, Jan Beulich wrote: >> This is a prereq to us, in particular, respecting the "ATC required" >> flag. >> >> Note that ACPI_SATC_ATC_REQUIRED has its #define put in dmar.h, as we >> try to keep actbl*.h in sync what Linux (who in turn inherit from ACPI >> CA) has. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> Lovely: On the SPR system with the SATC I tried passing "ats" (the >> "required" flag is clear there), just to then hit "IOMMU#4: QI dev wait >> descriptor taking too long" while setting up Dom0. The 2nd message there >> doesn't ever appear, so the request never completes. Not sure whether >> that's us doing something wrong or the hardware acting up. In the former >> case I'd generally expect an IOMMU fault to be raised, though. FTR same >> on 4.18 with just "VT-d: correct ATS checking for root complex >> integrated devices" backported there. > > Great, so we likely have a bug in our ATS implementation? Or there's a hardware / firmware issue. As said in the remark, while I'm not really sure which one it is, I'd kind of expect some form of error indication rather than just a hang if we did something wrong. >> --- a/xen/drivers/passthrough/vtd/dmar.c >> +++ b/xen/drivers/passthrough/vtd/dmar.c >> @@ -47,6 +47,7 @@ LIST_HEAD_READ_MOSTLY(acpi_drhd_units); >> LIST_HEAD_READ_MOSTLY(acpi_rmrr_units); >> static LIST_HEAD_READ_MOSTLY(acpi_atsr_units); >> static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units); >> +static LIST_HEAD_READ_MOSTLY(acpi_satc_units); > > We could even make this one RO after init. Maybe, after first introducing LIST_HEAD_RO_AFTER_INIT() and then perhaps switching the others up front. IOW I'd prefer to keep those consistent and then (if so desired) update them all in one go. >> @@ -750,6 +751,93 @@ acpi_parse_one_rhsa(struct acpi_dmar_hea >> return ret; >> } >> >> +static int __init register_one_satc(struct acpi_satc_unit *satcu) >> +{ >> + bool ignore = false; >> + unsigned int i = 0; >> + int ret = 0; >> + >> + /* Skip checking if segment is not accessible yet. */ >> + if ( !pci_known_segment(satcu->segment) ) >> + i = UINT_MAX; >> + >> + for ( ; i < satcu->scope.devices_cnt; i++ ) >> + { >> + uint8_t b = PCI_BUS(satcu->scope.devices[i]); >> + uint8_t d = PCI_SLOT(satcu->scope.devices[i]); >> + uint8_t f = PCI_FUNC(satcu->scope.devices[i]); >> + >> + if ( !pci_device_detect(satcu->segment, b, d, f) ) >> + { >> + dprintk(XENLOG_WARNING VTDPREFIX, >> + " Non-existent device (%pp) is reported in SATC scope!\n", >> + &PCI_SBDF(satcu->segment, b, d, f)); >> + ignore = true; >> + } >> + else >> + { >> + ignore = false; >> + break; >> + } >> + } >> + >> + if ( ignore ) >> + { >> + dprintk(XENLOG_WARNING VTDPREFIX, >> + " Ignore SATC for seg %04x as no device under its scope is PCI discoverable\n", >> + satcu->segment); > > Re the error messages: won't it be better to print them using plain > printk and gate on iommu_verbose being enabled if anything? > > It does seem a bit odd that such messages won't be printed when > iommu={debug,verbose} is enabled on the command line. Well, perhaps yes. Yet I'm trying here to stay (largely) in sync with how in particular register_one_rmrr() behaves. Do you strictly think I should diverge here? >> +static int __init >> +acpi_parse_one_satc(const struct acpi_dmar_header *header) >> +{ >> + const struct acpi_dmar_satc *satc = >> + container_of(header, const struct acpi_dmar_satc, header); >> + struct acpi_satc_unit *satcu; >> + const void *dev_scope_start, *dev_scope_end; >> + int ret = acpi_dmar_check_length(header, sizeof(*satc)); >> + >> + if ( ret ) >> + return ret; >> + >> + satcu = xzalloc(struct acpi_satc_unit); >> + if ( !satcu ) >> + return -ENOMEM; >> + >> + satcu->segment = satc->segment; >> + satcu->atc_required = satc->flags & ACPI_SATC_ATC_REQUIRED; >> + >> + dev_scope_start = (const void *)(satc + 1); >> + dev_scope_end = (const void *)satc + header->length; > > Isn't it enough to just cast to void * and inherit the const from the > left side variable declaration? Misra won't like the (transient) removal of const, afaict. Personally I also consider it bad practice to omit such const. > You could even initialize dev_scope_{start,end} at definition. Right. This is again the way it is to be in sync with other acpi_parse_one_...() functions. It's always hard to judge where to diverge and where consistency is weighed higher. Whichever way you do it, you may get comment asking for the opposite ... Jan
On Mon, May 06, 2024 at 01:01:48PM +0200, Jan Beulich wrote: > On 06.05.2024 12:29, Roger Pau Monné wrote: > > On Thu, Feb 15, 2024 at 11:14:31AM +0100, Jan Beulich wrote: > >> This is a prereq to us, in particular, respecting the "ATC required" > >> flag. > >> > >> Note that ACPI_SATC_ATC_REQUIRED has its #define put in dmar.h, as we > >> try to keep actbl*.h in sync what Linux (who in turn inherit from ACPI > >> CA) has. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> I think however it should be mentioned in the description that introduced code attempts to stay in sync with the existing register_one_satc and acpi_parse_one_*() functions. > >> --- > >> Lovely: On the SPR system with the SATC I tried passing "ats" (the > >> "required" flag is clear there), just to then hit "IOMMU#4: QI dev wait > >> descriptor taking too long" while setting up Dom0. The 2nd message there > >> doesn't ever appear, so the request never completes. Not sure whether > >> that's us doing something wrong or the hardware acting up. In the former > >> case I'd generally expect an IOMMU fault to be raised, though. FTR same > >> on 4.18 with just "VT-d: correct ATS checking for root complex > >> integrated devices" backported there. > > > > Great, so we likely have a bug in our ATS implementation? > > Or there's a hardware / firmware issue. As said in the remark, while I'm > not really sure which one it is, I'd kind of expect some form of error > indication rather than just a hang if we did something wrong. > > >> --- a/xen/drivers/passthrough/vtd/dmar.c > >> +++ b/xen/drivers/passthrough/vtd/dmar.c > >> @@ -47,6 +47,7 @@ LIST_HEAD_READ_MOSTLY(acpi_drhd_units); > >> LIST_HEAD_READ_MOSTLY(acpi_rmrr_units); > >> static LIST_HEAD_READ_MOSTLY(acpi_atsr_units); > >> static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units); > >> +static LIST_HEAD_READ_MOSTLY(acpi_satc_units); > > > > We could even make this one RO after init. > > Maybe, after first introducing LIST_HEAD_RO_AFTER_INIT() and then > perhaps switching the others up front. IOW I'd prefer to keep those > consistent and then (if so desired) update them all in one go. > > >> @@ -750,6 +751,93 @@ acpi_parse_one_rhsa(struct acpi_dmar_hea > >> return ret; > >> } > >> > >> +static int __init register_one_satc(struct acpi_satc_unit *satcu) > >> +{ > >> + bool ignore = false; > >> + unsigned int i = 0; > >> + int ret = 0; > >> + > >> + /* Skip checking if segment is not accessible yet. */ > >> + if ( !pci_known_segment(satcu->segment) ) > >> + i = UINT_MAX; > >> + > >> + for ( ; i < satcu->scope.devices_cnt; i++ ) > >> + { > >> + uint8_t b = PCI_BUS(satcu->scope.devices[i]); > >> + uint8_t d = PCI_SLOT(satcu->scope.devices[i]); > >> + uint8_t f = PCI_FUNC(satcu->scope.devices[i]); > >> + > >> + if ( !pci_device_detect(satcu->segment, b, d, f) ) > >> + { > >> + dprintk(XENLOG_WARNING VTDPREFIX, > >> + " Non-existent device (%pp) is reported in SATC scope!\n", > >> + &PCI_SBDF(satcu->segment, b, d, f)); > >> + ignore = true; > >> + } > >> + else > >> + { > >> + ignore = false; > >> + break; > >> + } > >> + } > >> + > >> + if ( ignore ) > >> + { > >> + dprintk(XENLOG_WARNING VTDPREFIX, > >> + " Ignore SATC for seg %04x as no device under its scope is PCI discoverable\n", > >> + satcu->segment); > > > > Re the error messages: won't it be better to print them using plain > > printk and gate on iommu_verbose being enabled if anything? > > > > It does seem a bit odd that such messages won't be printed when > > iommu={debug,verbose} is enabled on the command line. > > Well, perhaps yes. Yet I'm trying here to stay (largely) in sync with how > in particular register_one_rmrr() behaves. Do you strictly think I should > diverge here? > > >> +static int __init > >> +acpi_parse_one_satc(const struct acpi_dmar_header *header) > >> +{ > >> + const struct acpi_dmar_satc *satc = > >> + container_of(header, const struct acpi_dmar_satc, header); > >> + struct acpi_satc_unit *satcu; > >> + const void *dev_scope_start, *dev_scope_end; > >> + int ret = acpi_dmar_check_length(header, sizeof(*satc)); > >> + > >> + if ( ret ) > >> + return ret; > >> + > >> + satcu = xzalloc(struct acpi_satc_unit); > >> + if ( !satcu ) > >> + return -ENOMEM; > >> + > >> + satcu->segment = satc->segment; > >> + satcu->atc_required = satc->flags & ACPI_SATC_ATC_REQUIRED; > >> + > >> + dev_scope_start = (const void *)(satc + 1); > >> + dev_scope_end = (const void *)satc + header->length; > > > > Isn't it enough to just cast to void * and inherit the const from the > > left side variable declaration? > > Misra won't like the (transient) removal of const, afaict. Personally I > also consider it bad practice to omit such const. > > > You could even initialize dev_scope_{start,end} at definition. > > Right. This is again the way it is to be in sync with other > acpi_parse_one_...() functions. It's always hard to judge where to diverge > and where consistency is weighed higher. Whichever way you do it, you may > get comment asking for the opposite ... Oh, yes, IIRC you already mentioned this in v1, yet I've forgot when reviewing this one. Thanks, Roger.
--- a/xen/drivers/passthrough/vtd/dmar.c +++ b/xen/drivers/passthrough/vtd/dmar.c @@ -47,6 +47,7 @@ LIST_HEAD_READ_MOSTLY(acpi_drhd_units); LIST_HEAD_READ_MOSTLY(acpi_rmrr_units); static LIST_HEAD_READ_MOSTLY(acpi_atsr_units); static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units); +static LIST_HEAD_READ_MOSTLY(acpi_satc_units); static struct acpi_table_header *__read_mostly dmar_table; static int __read_mostly dmar_flags; @@ -750,6 +751,93 @@ acpi_parse_one_rhsa(struct acpi_dmar_hea return ret; } +static int __init register_one_satc(struct acpi_satc_unit *satcu) +{ + bool ignore = false; + unsigned int i = 0; + int ret = 0; + + /* Skip checking if segment is not accessible yet. */ + if ( !pci_known_segment(satcu->segment) ) + i = UINT_MAX; + + for ( ; i < satcu->scope.devices_cnt; i++ ) + { + uint8_t b = PCI_BUS(satcu->scope.devices[i]); + uint8_t d = PCI_SLOT(satcu->scope.devices[i]); + uint8_t f = PCI_FUNC(satcu->scope.devices[i]); + + if ( !pci_device_detect(satcu->segment, b, d, f) ) + { + dprintk(XENLOG_WARNING VTDPREFIX, + " Non-existent device (%pp) is reported in SATC scope!\n", + &PCI_SBDF(satcu->segment, b, d, f)); + ignore = true; + } + else + { + ignore = false; + break; + } + } + + if ( ignore ) + { + dprintk(XENLOG_WARNING VTDPREFIX, + " Ignore SATC for seg %04x as no device under its scope is PCI discoverable\n", + satcu->segment); + return 1; + } + + if ( iommu_verbose ) + printk(VTDPREFIX " ATC required: %d\n", satcu->atc_required); + + list_add(&satcu->list, &acpi_satc_units); + + return ret; +} + +static int __init +acpi_parse_one_satc(const struct acpi_dmar_header *header) +{ + const struct acpi_dmar_satc *satc = + container_of(header, const struct acpi_dmar_satc, header); + struct acpi_satc_unit *satcu; + const void *dev_scope_start, *dev_scope_end; + int ret = acpi_dmar_check_length(header, sizeof(*satc)); + + if ( ret ) + return ret; + + satcu = xzalloc(struct acpi_satc_unit); + if ( !satcu ) + return -ENOMEM; + + satcu->segment = satc->segment; + satcu->atc_required = satc->flags & ACPI_SATC_ATC_REQUIRED; + + dev_scope_start = (const void *)(satc + 1); + dev_scope_end = (const void *)satc + header->length; + ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end, + &satcu->scope, SATC_TYPE, satc->segment); + + if ( !ret && satcu->scope.devices_cnt ) + ret = register_one_satc(satcu); + + if ( ret ) + { + scope_devices_free(&satcu->scope); + xfree(satcu); + } + + /* + * register_one_satc() returns greater than 0 when a specified PCIe + * device cannot be detected. To prevent VT-d from being disabled in + * such cases, make the return value 0 here. + */ + return ret > 0 ? 0 : ret; +} + static int __init cf_check acpi_parse_dmar(struct acpi_table_header *table) { struct acpi_table_dmar *dmar; @@ -803,6 +891,13 @@ static int __init cf_check acpi_parse_dm printk(VTDPREFIX "found ACPI_DMAR_RHSA:\n"); ret = acpi_parse_one_rhsa(entry_header); break; + + case ACPI_DMAR_TYPE_SATC: + if ( iommu_verbose ) + printk(VTDPREFIX "found ACPI_DMAR_SATC:\n"); + ret = acpi_parse_one_satc(entry_header); + break; + default: dprintk(XENLOG_WARNING VTDPREFIX, "Ignore unknown DMAR structure type (%#x)\n", --- a/xen/drivers/passthrough/vtd/dmar.h +++ b/xen/drivers/passthrough/vtd/dmar.h @@ -91,6 +91,16 @@ struct acpi_rhsa_unit { u32 proximity_domain; }; +struct acpi_satc_unit { + struct dmar_scope scope; + struct list_head list; + uint16_t segment; + bool atc_required:1; +}; + +/* In lieu of a definition in actbl2.h. */ +#define ACPI_SATC_ATC_REQUIRED (1U << 0) + #define for_each_drhd_unit(drhd) \ list_for_each_entry(drhd, &acpi_drhd_units, list) @@ -106,6 +116,7 @@ struct acpi_atsr_unit *acpi_find_matched #define DMAR_TYPE 1 #define RMRR_TYPE 2 #define ATSR_TYPE 3 +#define SATC_TYPE 4 #define DMAR_OPERATION_TIMEOUT MILLISECS(1000) --- a/xen/include/acpi/actbl2.h +++ b/xen/include/acpi/actbl2.h @@ -345,7 +345,8 @@ enum acpi_dmar_type { ACPI_DMAR_TYPE_RESERVED_MEMORY = 1, ACPI_DMAR_TYPE_ATSR = 2, ACPI_DMAR_HARDWARE_AFFINITY = 3, - ACPI_DMAR_TYPE_RESERVED = 4 /* 4 and greater are reserved */ + ACPI_DMAR_TYPE_SATC = 5, + ACPI_DMAR_TYPE_RESERVED = 7 /* 7 and greater are reserved */ }; /* DMAR Device Scope structure */ @@ -427,6 +428,15 @@ struct acpi_dmar_rhsa { u32 proximity_domain; }; +/* 5: SOC Integrated Address Translation Cache Reporting Structure */ + +struct acpi_dmar_satc { + struct acpi_dmar_header header; + uint8_t flags; + uint8_t reserved; + uint16_t segment; +}; + /******************************************************************************* * * HPET - High Precision Event Timer table
This is a prereq to us, in particular, respecting the "ATC required" flag. Note that ACPI_SATC_ATC_REQUIRED has its #define put in dmar.h, as we try to keep actbl*.h in sync what Linux (who in turn inherit from ACPI CA) has. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Lovely: On the SPR system with the SATC I tried passing "ats" (the "required" flag is clear there), just to then hit "IOMMU#4: QI dev wait descriptor taking too long" while setting up Dom0. The 2nd message there doesn't ever appear, so the request never completes. Not sure whether that's us doing something wrong or the hardware acting up. In the former case I'd generally expect an IOMMU fault to be raised, though. FTR same on 4.18 with just "VT-d: correct ATS checking for root complex integrated devices" backported there. Should we check scope entries for appropriate types? (If so, then also for e.g. ATSR.) --- v2: Move error case freeing to acpi_parse_one_satc(). Introduce #define for the flag bit. Style.