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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> 
> --- 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.