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

[Jan Beulich <jbeulich@xxxxxxxx>]

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