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

["Roger Pau Monné" <roger.pau@citrix.com>]

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@suse.com>
> ---
> 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?

> 
> Should we check scope entries for appropriate types? (If so, then also
> for e.g. ATSR.)
> ---
> v2: Move error case freeing to acpi_parse_one_satc(). Introduce #define
>     for the flag bit. Style.
> 
> --- 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.

>  
>  static struct acpi_table_header *__read_mostly dmar_table;
>  static int __read_mostly dmar_flags;
> @@ -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.

> +        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 = 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?

You could even initialize dev_scope_{start,end} at definition.

Thanks, Roger.