Re: [PATCH 1/7] VT-d: parse ACPI "SoC Integrated Address Translation Cache Reporting Structure"s

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>> ---
>> 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