| Message ID | b5a58dee-9a4c-4833-be59-b52c62f7137d@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | VT-d: SATC handling and ATS tidying | expand |
On Mon, Feb 05, 2024 at 02:55:17PM +0100, Jan Beulich wrote: > This is a prereq to us, in particular, respecting the "ATC required" > flag. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Should we check scope entries for appropriate types? (If so, then also > for e.g. ATSR.) Hm, I guess we could do so in acpi_parse_dev_scope() since that function already gets passed a 'type' argument. > > --- 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; > @@ -764,6 +765,95 @@ 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) == 0 ) Any reason to explicitly compare against 0? if ( !pci_device_detect(satcu->segment, b, d, f) ) ... The function returns a boolean. > + { > + dprintk(XENLOG_WARNING VTDPREFIX, > + " Non-existent device (%pp) is reported in SATC scope!\n", > + &PCI_SBDF(satcu->segment, b, d, f)); > + ignore = true; This is kind of reporting is incomplete: as soon as one device is found the loop is exited and no further detection happens. If we want to print such information, we should do the full scan and avoid exiting early when a populated device is detected. > + } > + 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", (I would drop the '!' at the end, here and above, I don't think they add much to the error message) I would also use the '#' flag to avoid confusion, as in the weird case we have a segment '1234' then without the zero padding I wouldn't really know whether it's decimal or hex. > + satcu->segment); > + scope_devices_free(&satcu->scope); > + xfree(satcu); > + 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; > + > + if ( (ret = acpi_dmar_check_length(header, sizeof(*satc))) != 0 ) > + return ret; A very stupid nit, but I would rather prefer: int ret = acpi_dmar_check_length(header, sizeof(*satc)); if ( ret ) return ret; Which has the same number of lines and is IMO easier to read. > + > + satcu = xzalloc(struct acpi_satc_unit); > + if ( !satcu ) > + return -ENOMEM; > + > + satcu->segment = satc->segment; > + satcu->atc_required = satc->flags & 1; I would add this as a define in actbl2.h: #define ACPI_DMAR_ATC_REQUIRED (1U << 0) Or some such (maybe just using plain 1 is also fine). > + > + 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); > + /* > + * 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, reset the return value to 0 here. > + */ > + if ( ret > 0 ) > + ret = 0; > + } > + else > + xfree(satcu); You can safely use scope_devices_free() even if acpi_parse_dev_scope() failed, so you could unify the freeing here, instead of doing it in register_one_satc() also. Also why not make register_one_satc() return a boolean instead of 0/1? if ( ret || !satcu->scope.devices_cnt || register_one_satc(satcu) ) { scope_devices_free(&satcu->scope); xfree(satcu); } > + > + return ret; > +} > + > static int __init cf_check acpi_parse_dmar(struct acpi_table_header *table) > { > struct acpi_table_dmar *dmar; > @@ -817,6 +907,11 @@ 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; I know the surrounding code doesn't use it, but readability would benefit from adding a blank line after the break statement. Thanks, Roger.
On 08.02.2024 10:17, Roger Pau Monné wrote: > On Mon, Feb 05, 2024 at 02:55:17PM +0100, Jan Beulich wrote: >> This is a prereq to us, in particular, respecting the "ATC required" >> flag. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> Should we check scope entries for appropriate types? (If so, then also >> for e.g. ATSR.) > > Hm, I guess we could do so in acpi_parse_dev_scope() since that > function already gets passed a 'type' argument. Right, I transiently had it there, but then dropped it again for being inconsistent with what we have right now. I'll try to remember to add another patch. >> @@ -764,6 +765,95 @@ 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) == 0 ) > > Any reason to explicitly compare against 0? > > if ( !pci_device_detect(satcu->segment, b, d, f) ) > ... > > The function returns a boolean. Hmm, right - simply a result of copy-and-paste. >> + { >> + dprintk(XENLOG_WARNING VTDPREFIX, >> + " Non-existent device (%pp) is reported in SATC scope!\n", >> + &PCI_SBDF(satcu->segment, b, d, f)); >> + ignore = true; > > This is kind of reporting is incomplete: as soon as one device is > found the loop is exited and no further detection happens. If we want > to print such information, we should do the full scan and avoid > exiting early when a populated device is detected. Not sure I follow, but first of all - these are dprintk()s only, so meant to only help in dev environments. Specifically ... >> + } >> + 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", ... this message is then issued only bogus entries were found. IOW when a real device was found, there's no real reason to report N other bogus ones, I think. Plus, whatever we change here ought to also / first change in register_one_rmrr(). > (I would drop the '!' at the end, here and above, I don't think they > add much to the error message) Sure; copy-and-paste again. > I would also use the '#' flag to avoid > confusion, as in the weird case we have a segment '1234' then without > the zero padding I wouldn't really know whether it's decimal or hex. Not really, no. If there's any place we log segment numbers in decimal, we should change that. They ought to be possible to match with the usual ssss:bb:dd.f coordinates we log. >> + satcu = xzalloc(struct acpi_satc_unit); >> + if ( !satcu ) >> + return -ENOMEM; >> + >> + satcu->segment = satc->segment; >> + satcu->atc_required = satc->flags & 1; > > I would add this as a define in actbl2.h: > > #define ACPI_DMAR_ATC_REQUIRED (1U << 0) > > Or some such (maybe just using plain 1 is also fine). I intended to do so, but strictly staying in line with what Linux has. To my surprise they use a literal number and have no #define. Hence I didn't add any either. >> + >> + 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); >> + /* >> + * 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, reset the return value to 0 here. >> + */ >> + if ( ret > 0 ) >> + ret = 0; >> + } >> + else >> + xfree(satcu); > > You can safely use scope_devices_free() even if acpi_parse_dev_scope() > failed, so you could unify the freeing here, instead of doing it in > register_one_satc() also. Moving that out of acpi_parse_dev_scope() would feel wrong - if a function fails, it would better not leave cleanup to its caller(s). > Also why not make register_one_satc() return a boolean instead of 0/1? To leave room to also return errors, like register_one_rmrr() does. For both of these aspects you raise: I'd really like to avoid these sibling functions going too much out of sync. >> @@ -817,6 +907,11 @@ 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; > > I know the surrounding code doesn't use it, but readability would > benefit from adding a blank line after the break statement. Well, yes, done so. I'm not generally in favor of introducing such inconsistencies, but it looks tolerable here. (In cases like this I do - and did here - consider this as an option, but in most cases I end up valuing uniform look slightly higher.) Jan
On Thu, Feb 08, 2024 at 04:29:34PM +0100, Jan Beulich wrote: > On 08.02.2024 10:17, Roger Pau Monné wrote: > > On Mon, Feb 05, 2024 at 02:55:17PM +0100, Jan Beulich wrote: > >> This is a prereq to us, in particular, respecting the "ATC required" > >> flag. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> Should we check scope entries for appropriate types? (If so, then also > >> for e.g. ATSR.) > > > > Hm, I guess we could do so in acpi_parse_dev_scope() since that > > function already gets passed a 'type' argument. > > Right, I transiently had it there, but then dropped it again for being > inconsistent with what we have right now. I'll try to remember to add > another patch. No strict requirement - but since it's on the spec we might as well try to honor it. > >> + { > >> + dprintk(XENLOG_WARNING VTDPREFIX, > >> + " Non-existent device (%pp) is reported in SATC scope!\n", > >> + &PCI_SBDF(satcu->segment, b, d, f)); > >> + ignore = true; > > > > This is kind of reporting is incomplete: as soon as one device is > > found the loop is exited and no further detection happens. If we want > > to print such information, we should do the full scan and avoid > > exiting early when a populated device is detected. > > Not sure I follow, but first of all - these are dprintk()s only, so > meant to only help in dev environments. Specifically ... > > >> + } > >> + 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", > > ... this message is then issued only bogus entries were found. IOW > when a real device was found, there's no real reason to report N > other bogus ones, I think. I guess it's a question of taste. I do find it odd (asymmetric maybe?) that we stop reporting non-existing devices once a valid device is found. Makes me wonder what's the point of reporting them in the first place, if the list of non-existing devices is not complete? > Plus, whatever we change here ought to also / first change in > register_one_rmrr(). I could live with those looking differently, or register_one_rmrr() can be adjusted later. Existing examples shouldn't be an argument to not make new additions better. But that's only if you agree the suggested changes make the code better, if you prefer the current implementation then there's no point in arguing whether we should keep register_one_rmrr() and the newly introduce function similar enough. > >> + satcu = xzalloc(struct acpi_satc_unit); > >> + if ( !satcu ) > >> + return -ENOMEM; > >> + > >> + satcu->segment = satc->segment; > >> + satcu->atc_required = satc->flags & 1; > > > > I would add this as a define in actbl2.h: > > > > #define ACPI_DMAR_ATC_REQUIRED (1U << 0) > > > > Or some such (maybe just using plain 1 is also fine). > > I intended to do so, but strictly staying in line with what Linux has. > To my surprise they use a literal number and have no #define. Hence I > didn't add any either. I would prefer the define unless you have strong objections, even if that means diverging from Linux. > >> + > >> + 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); > >> + /* > >> + * 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, reset the return value to 0 here. > >> + */ > >> + if ( ret > 0 ) > >> + ret = 0; > >> + } > >> + else > >> + xfree(satcu); > > > > You can safely use scope_devices_free() even if acpi_parse_dev_scope() > > failed, so you could unify the freeing here, instead of doing it in > > register_one_satc() also. > > Moving that out of acpi_parse_dev_scope() would feel wrong - if a > function fails, it would better not leave cleanup to its caller(s). Given that the caller here is the one that did the allocation my preference would be to also do the cleanup there - register_one_satc() has no need to know what needs freeing, and allows unifying the cleanup in a single place. > > Also why not make register_one_satc() return a boolean instead of 0/1? > > To leave room to also return errors, like register_one_rmrr() does. > > For both of these aspects you raise: I'd really like to avoid these > sibling functions going too much out of sync. I could live with those going out-of-sync (or adjusted later), but only if you think the suggestions are an improvement. > >> @@ -817,6 +907,11 @@ 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; > > > > I know the surrounding code doesn't use it, but readability would > > benefit from adding a blank line after the break statement. > > Well, yes, done so. I'm not generally in favor of introducing such > inconsistencies, but it looks tolerable here. (In cases like this > I do - and did here - consider this as an option, but in most cases > I end up valuing uniform look slightly higher.) Yeah, overall I think it's an improvement if we go switching those as we modify the code for other reasons. Thanks, Roger.
On 09.02.2024 10:00, Roger Pau Monné wrote: > On Thu, Feb 08, 2024 at 04:29:34PM +0100, Jan Beulich wrote: >> On 08.02.2024 10:17, Roger Pau Monné wrote: >>> On Mon, Feb 05, 2024 at 02:55:17PM +0100, Jan Beulich wrote: >>>> + { >>>> + dprintk(XENLOG_WARNING VTDPREFIX, >>>> + " Non-existent device (%pp) is reported in SATC scope!\n", >>>> + &PCI_SBDF(satcu->segment, b, d, f)); >>>> + ignore = true; >>> >>> This is kind of reporting is incomplete: as soon as one device is >>> found the loop is exited and no further detection happens. If we want >>> to print such information, we should do the full scan and avoid >>> exiting early when a populated device is detected. >> >> Not sure I follow, but first of all - these are dprintk()s only, so >> meant to only help in dev environments. Specifically ... >> >>>> + } >>>> + 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", >> >> ... this message is then issued only bogus entries were found. IOW >> when a real device was found, there's no real reason to report N >> other bogus ones, I think. > > I guess it's a question of taste. I do find it odd (asymmetric > maybe?) that we stop reporting non-existing devices once a valid > device is found. Makes me wonder what's the point of reporting them > in the first place, if the list of non-existing devices is not > complete? Since you look to not be taking this into account, let me re-emphasize that these are dprintk() only. In the event of an issue, seeing the log messages you at least get a hint of one device that poses a problem. That may or may not be enough of an indication for figuring what's wrong. Making the loop run for longer than necessary when especially in a release build there's not going to be any change (but the logic would become [slightly] more complex, as after setting "ignore" to true we'd need to avoid clearing it back to false) is just pointless imo. IOW I view this 1st message as merely a courtesy for the case where the 2nd one would end up also being logged. >> Plus, whatever we change here ought to also / first change in >> register_one_rmrr(). > > I could live with those looking differently, or register_one_rmrr() > can be adjusted later. Existing examples shouldn't be an argument to > not make new additions better. While I generally agree with this principle, in cases like this one it needs weighing against consistency. Which I consider more important here, to reduce the chance of making mistakes when fiddling with this code later again. >>>> + satcu = xzalloc(struct acpi_satc_unit); >>>> + if ( !satcu ) >>>> + return -ENOMEM; >>>> + >>>> + satcu->segment = satc->segment; >>>> + satcu->atc_required = satc->flags & 1; >>> >>> I would add this as a define in actbl2.h: >>> >>> #define ACPI_DMAR_ATC_REQUIRED (1U << 0) >>> >>> Or some such (maybe just using plain 1 is also fine). >> >> I intended to do so, but strictly staying in line with what Linux has. >> To my surprise they use a literal number and have no #define. Hence I >> didn't add any either. > > I would prefer the define unless you have strong objections, even if > that means diverging from Linux. I could probably accept such a #define living in one of dmar.[ch]. I'd rather not see it go into actbl2.h. >>>> + >>>> + 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); >>>> + /* >>>> + * 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, reset the return value to 0 here. >>>> + */ >>>> + if ( ret > 0 ) >>>> + ret = 0; >>>> + } >>>> + else >>>> + xfree(satcu); >>> >>> You can safely use scope_devices_free() even if acpi_parse_dev_scope() >>> failed, so you could unify the freeing here, instead of doing it in >>> register_one_satc() also. >> >> Moving that out of acpi_parse_dev_scope() would feel wrong - if a >> function fails, it would better not leave cleanup to its caller(s). > > Given that the caller here is the one that did the allocation my > preference would be to also do the cleanup there - register_one_satc() > has no need to know what needs freeing, and allows unifying the > cleanup in a single place. Hmm, you're right about the odd freeing behavior. I guess I really ought to change that, but then first for register_one_rmrr() (seeing that DRHD and ATSR parsing also do it that way). Which then of course means also touching add_one_user_rmrr(). Jan
On Mon, Feb 12, 2024 at 10:32:00AM +0100, Jan Beulich wrote: > On 09.02.2024 10:00, Roger Pau Monné wrote: > > On Thu, Feb 08, 2024 at 04:29:34PM +0100, Jan Beulich wrote: > >> On 08.02.2024 10:17, Roger Pau Monné wrote: > >>> On Mon, Feb 05, 2024 at 02:55:17PM +0100, Jan Beulich wrote: > >>>> + { > >>>> + dprintk(XENLOG_WARNING VTDPREFIX, > >>>> + " Non-existent device (%pp) is reported in SATC scope!\n", > >>>> + &PCI_SBDF(satcu->segment, b, d, f)); > >>>> + ignore = true; > >>> > >>> This is kind of reporting is incomplete: as soon as one device is > >>> found the loop is exited and no further detection happens. If we want > >>> to print such information, we should do the full scan and avoid > >>> exiting early when a populated device is detected. > >> > >> Not sure I follow, but first of all - these are dprintk()s only, so > >> meant to only help in dev environments. Specifically ... > >> > >>>> + } > >>>> + 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", > >> > >> ... this message is then issued only bogus entries were found. IOW > >> when a real device was found, there's no real reason to report N > >> other bogus ones, I think. > > > > I guess it's a question of taste. I do find it odd (asymmetric > > maybe?) that we stop reporting non-existing devices once a valid > > device is found. Makes me wonder what's the point of reporting them > > in the first place, if the list of non-existing devices is not > > complete? > > Since you look to not be taking this into account, let me re-emphasize > that these are dprintk() only. In the event of an issue, seeing the > log messages you at least get a hint of one device that poses a > problem. That may or may not be enough of an indication for figuring > what's wrong. Making the loop run for longer than necessary when > especially in a release build there's not going to be any change (but > the logic would become [slightly] more complex, as after setting > "ignore" to true we'd need to avoid clearing it back to false) is just > pointless imo. IOW I view this 1st message as merely a courtesy for > the case where the 2nd one would end up also being logged. I will not insist anymore. > >>>> + satcu = xzalloc(struct acpi_satc_unit); > >>>> + if ( !satcu ) > >>>> + return -ENOMEM; > >>>> + > >>>> + satcu->segment = satc->segment; > >>>> + satcu->atc_required = satc->flags & 1; > >>> > >>> I would add this as a define in actbl2.h: > >>> > >>> #define ACPI_DMAR_ATC_REQUIRED (1U << 0) > >>> > >>> Or some such (maybe just using plain 1 is also fine). > >> > >> I intended to do so, but strictly staying in line with what Linux has. > >> To my surprise they use a literal number and have no #define. Hence I > >> didn't add any either. > > > > I would prefer the define unless you have strong objections, even if > > that means diverging from Linux. > > I could probably accept such a #define living in one of dmar.[ch]. I'd > rather not see it go into actbl2.h. Fine. I think the current open coding of 1 in Linux is wrong. Other flag fields in DMAR structures have the related defines. 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; @@ -764,6 +765,95 @@ 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) == 0 ) + { + 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); + scope_devices_free(&satcu->scope); + xfree(satcu); + 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; + + if ( (ret = acpi_dmar_check_length(header, sizeof(*satc))) != 0 ) + return ret; + + satcu = xzalloc(struct acpi_satc_unit); + if ( !satcu ) + return -ENOMEM; + + satcu->segment = satc->segment; + satcu->atc_required = satc->flags & 1; + + 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); + /* + * 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, reset the return value to 0 here. + */ + if ( ret > 0 ) + ret = 0; + } + else + xfree(satcu); + + return ret; +} + static int __init cf_check acpi_parse_dmar(struct acpi_table_header *table) { struct acpi_table_dmar *dmar; @@ -817,6 +907,11 @@ 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,13 @@ 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; +}; + #define for_each_drhd_unit(drhd) \ list_for_each_entry(drhd, &acpi_drhd_units, list) @@ -106,6 +113,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. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Should we check scope entries for appropriate types? (If so, then also for e.g. ATSR.)