Re: [Xen-devel] [PATCH 2/9] AMD/IOMMU: use bit field for control register

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/06/2019 14:22, Jan Beulich wrote:
> Also introduce a field in struct amd_iommu caching the most recently
> written control register. All writes should now happen exclusively from
> that cached value, such that it is guaranteed to be up to date.
>
> Take the opportunity and add further fields. Also convert a few boolean
> function parameters to bool, such that use of !! can be avoided.

Critically also, some previous writel()'s have turned into writeq(),
which needs calling out.

> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -69,31 +69,18 @@ static void __init unmap_iommu_mmio_regi
>  
>  static void set_iommu_ht_flags(struct amd_iommu *iommu)
>  {
> -    u32 entry;
> -    entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
> -
>      /* Setup HT flags */
>      if ( iommu_has_cap(iommu, PCI_CAP_HT_TUNNEL_SHIFT) )
> -        iommu_has_ht_flag(iommu, ACPI_IVHD_TT_ENABLE) ?
> -            iommu_set_bit(&entry, IOMMU_CONTROL_HT_TUNNEL_TRANSLATION_SHIFT) 
> :
> -            iommu_clear_bit(&entry, 
> IOMMU_CONTROL_HT_TUNNEL_TRANSLATION_SHIFT);
> -
> -    iommu_has_ht_flag(iommu, ACPI_IVHD_RES_PASS_PW) ?
> -        iommu_set_bit(&entry, IOMMU_CONTROL_RESP_PASS_POSTED_WRITE_SHIFT):
> -        iommu_clear_bit(&entry, IOMMU_CONTROL_RESP_PASS_POSTED_WRITE_SHIFT);
> -
> -    iommu_has_ht_flag(iommu, ACPI_IVHD_ISOC) ?
> -        iommu_set_bit(&entry, IOMMU_CONTROL_ISOCHRONOUS_SHIFT):
> -        iommu_clear_bit(&entry, IOMMU_CONTROL_ISOCHRONOUS_SHIFT);
> -
> -    iommu_has_ht_flag(iommu, ACPI_IVHD_PASS_PW) ?
> -        iommu_set_bit(&entry, IOMMU_CONTROL_PASS_POSTED_WRITE_SHIFT):
> -        iommu_clear_bit(&entry, IOMMU_CONTROL_PASS_POSTED_WRITE_SHIFT);
> +        iommu->ctrl.ht_tun_en = iommu_has_ht_flag(iommu, 
> ACPI_IVHD_TT_ENABLE);
> +
> +    iommu->ctrl.pass_pw     = iommu_has_ht_flag(iommu, ACPI_IVHD_PASS_PW);
> +    iommu->ctrl.res_pass_pw = iommu_has_ht_flag(iommu, 
> ACPI_IVHD_RES_PASS_PW);
> +    iommu->ctrl.isoc        = iommu_has_ht_flag(iommu, ACPI_IVHD_ISOC);
>  
>      /* Force coherent */
> -    iommu_set_bit(&entry, IOMMU_CONTROL_COHERENT_SHIFT);
> +    iommu->ctrl.coherent = 1;

Ah - so this is the AMD version of Intel's iommu=snoop

> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -295,38 +295,55 @@ struct amd_iommu_dte {
>
> +union amd_iommu_control {
> +    uint64_t raw;
> +    struct {
> +        unsigned int iommu_en:1;
> +        unsigned int ht_tun_en:1;
> +        unsigned int event_log_en:1;
> +        unsigned int event_int_en:1;
> +        unsigned int com_wait_int_en:1;
> +        unsigned int inv_timeout:3;
> +        unsigned int pass_pw:1;
> +        unsigned int res_pass_pw:1;
> +        unsigned int coherent:1;
> +        unsigned int isoc:1;
> +        unsigned int cmd_buf_en:1;
> +        unsigned int ppr_log_en:1;
> +        unsigned int ppr_int_en:1;
> +        unsigned int ppr_en:1;
> +        unsigned int gt_en:1;
> +        unsigned int ga_en:1;
> +        unsigned int crw:4;

This field does have an assigned name, but is also documented as Res0
for forwards compatibility.  I think this field wants handling
consistently with...

> +        unsigned int smif_en:1;
> +        unsigned int slf_wb_dis:1;
> +        unsigned int smif_log_en:1;
> +        unsigned int gam_en:3;
> +        unsigned int ga_log_en:1;
> +        unsigned int ga_int_en:1;
> +        unsigned int dual_ppr_log_en:2;
> +        unsigned int dual_event_log_en:2;
> +        unsigned int dev_tbl_seg_en:3;
> +        unsigned int priv_abrt_en:2;
> +        unsigned int ppr_auto_rsp_en:1;
> +        unsigned int marc_en:1;
> +        unsigned int blk_stop_mrk_en:1;
> +        unsigned int ppr_auto_rsp_aon:1;
> +        unsigned int :2;

... this, where you have dropped the DomainIDPNE bit (whatever the PN
stands for).

~Andrew

> +        unsigned int eph_en:1;
> +        unsigned int had_update:2;
> +        unsigned int gd_update_dis:1;
> +        unsigned int :1;
> +        unsigned int xt_en:1;
> +        unsigned int int_cap_xt_en:1;
> +        unsigned int vcmd_en:1;
> +        unsigned int viommu_en:1;
> +        unsigned int ga_update_dis:1;
> +        unsigned int gappi_en:1;
> +        unsigned int :8;
> +    };
> +};
>  
>  /* Exclusion Register */
>  #define IOMMU_EXCLUSION_BASE_LOW_OFFSET              0x20
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel