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
