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

[Jan Beulich <jbeulich@xxxxxxxx>]

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