[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 01/12] AMD/IOMMU: use bit field for extended feature register


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: "Woods, Brian" <Brian.Woods@xxxxxxx>
  • Date: Tue, 30 Jul 2019 16:35:48 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1;spf=pass smtp.mailfrom=amd.com;dmarc=pass action=none header.from=amd.com;dkim=pass header.d=amd.com;arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=bjpctvCNY1TVBX/CUS6uknZ7nSbvIvysjB18DGSAsUU=; b=KGbWA8x5nAi6JcxvWzXiCRbJrFTiHvH2xB+VvvEWfKScDuJmkvl+9cENZO0P3tIJunVUI1OCYqp/EKPvEYEypfCfQ5X3IdsaDLiAh6J9XvvTMwgtzAUVDkOlK79+vLpbTelYJ+KN2k6iiQXsMXanCfk+iT8z2X9YTbm8iYPUmditgGMIE6tx9TW63qlDfOqeKxkmLx110kd26vDVUoZDbze8TRN52jl29y8GHfT6sqLhLViQYtXb+CGiY0+yYJQD8BCB5hETK/cqFLxjpdpMLdN+bL5lPgH3Dy9uelJA3G1gj9ZWDVPkzO8sIjdUpNN0je0hYTG9fdSvEh2ZS2u3Rg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=h4nM6UJ7RQ1NpkOWgM5rwuiONBYhlWOXbxEy68wDkDIeZYIWaEyFHJvmY3CqYzlSFSzT5en8VxtlGcAgmzBLn1n2FJvqDgRB/K2TdPlJBsFPSbPf8CuogGlGKxI10OylWRzK4pjZR92d6GeYUKLtOJKIUsVSKBl6aLI8m0+GRVyAU0sEas82uOwkeZNpspPxErnWfRlMm0aIPap4OdF5fyh07s7LsI7HdN+bGITIK7XF3Oj957BIyAvrScjkZAl9XSzA3Hhz2T2xXqNTHh7bOZ2JVQCsKRUqGKWTs+wg2LjvdOMvIQx6lzX7PAJwlxyuIGIVmThaQMcN8GjdepZixA==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=Brian.Woods@xxxxxxx;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Woods, Brian" <Brian.Woods@xxxxxxx>, "Suthikulpanit, Suravee" <Suravee.Suthikulpanit@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Delivery-date: Tue, 30 Jul 2019 16:35:58 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVQu4aFQuwvyGZwUyEIomvAZIXdqbjY9AA
  • Thread-topic: [PATCH v4 01/12] AMD/IOMMU: use bit field for extended feature register

On Thu, Jul 25, 2019 at 01:29:16PM +0000, Jan Beulich wrote:
> This also takes care of several of the shift values wrongly having been
> specified as hex rather than dec.
> 
> Take the opportunity and
> - replace a readl() pair by a single readq(),
> - add further fields.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Acked-by: Brian Woods <brian.woods@xxxxxxx>

> ---
> v4: Drop stray/leftover #undef.
> v3: Another attempt at deriving masks from bitfields, hopefully better
>      liked by clang (mine was fine even with the v2 variant).
> v2: Correct sats_sup position and name. Re-base over new earlier patch.
> 
> --- a/xen/drivers/passthrough/amd/iommu_detect.c
> +++ b/xen/drivers/passthrough/amd/iommu_detect.c
> @@ -60,49 +60,76 @@ static int __init get_iommu_capabilities
>   
>   void __init get_iommu_features(struct amd_iommu *iommu)
>   {
> -    u32 low, high;
> -    int i = 0 ;
>       const struct amd_iommu *first;
> -    static const char *__initdata feature_str[] = {
> -        "- Prefetch Pages Command",
> -        "- Peripheral Page Service Request",
> -        "- X2APIC Supported",
> -        "- NX bit Supported",
> -        "- Guest Translation",
> -        "- Reserved bit [5]",
> -        "- Invalidate All Command",
> -        "- Guest APIC supported",
> -        "- Hardware Error Registers",
> -        "- Performance Counters",
> -        NULL
> -    };
> -
>       ASSERT( iommu->mmio_base );
>   
>       if ( !iommu_has_cap(iommu, PCI_CAP_EFRSUP_SHIFT) )
>       {
> -        iommu->features = 0;
> +        iommu->features.raw = 0;
>           return;
>       }
>   
> -    low = readl(iommu->mmio_base + IOMMU_EXT_FEATURE_MMIO_OFFSET);
> -    high = readl(iommu->mmio_base + IOMMU_EXT_FEATURE_MMIO_OFFSET + 4);
> -
> -    iommu->features = ((u64)high << 32) | low;
> +    iommu->features.raw =
> +        readq(iommu->mmio_base + IOMMU_EXT_FEATURE_MMIO_OFFSET);
>   
>       /* Don't log the same set of features over and over. */
>       first = list_first_entry(&amd_iommu_head, struct amd_iommu, list);
> -    if ( iommu != first && iommu->features == first->features )
> +    if ( iommu != first && iommu->features.raw == first->features.raw )
>           return;
>   
>       printk("AMD-Vi: IOMMU Extended Features:\n");
>   
> -    while ( feature_str[i] )
> +#define FEAT(fld, str) do {                                    \
> +    if ( --((union amd_iommu_ext_features){}).flds.fld > 1 )   \
> +        printk( "- " str ": %#x\n", iommu->features.flds.fld); \
> +    else if ( iommu->features.flds.fld )                       \
> +        printk( "- " str "\n");                                \
> +} while ( false )
> +
> +    FEAT(pref_sup,           "Prefetch Pages Command");
> +    FEAT(ppr_sup,            "Peripheral Page Service Request");
> +    FEAT(xt_sup,             "x2APIC");
> +    FEAT(nx_sup,             "NX bit");
> +    FEAT(gappi_sup,          "Guest APIC Physical Processor Interrupt");
> +    FEAT(ia_sup,             "Invalidate All Command");
> +    FEAT(ga_sup,             "Guest APIC");
> +    FEAT(he_sup,             "Hardware Error Registers");
> +    FEAT(pc_sup,             "Performance Counters");
> +    FEAT(hats,               "Host Address Translation Size");
> +
> +    if ( iommu->features.flds.gt_sup )
>       {
> -        if ( amd_iommu_has_feature(iommu, i) )
> -            printk( " %s\n", feature_str[i]);
> -        i++;
> +        FEAT(gats,           "Guest Address Translation Size");
> +        FEAT(glx_sup,        "Guest CR3 Root Table Level");
> +        FEAT(pas_max,        "Maximum PASID");
>       }
> +
> +    FEAT(smif_sup,           "SMI Filter Register");
> +    FEAT(smif_rc,            "SMI Filter Register Count");
> +    FEAT(gam_sup,            "Guest Virtual APIC Modes");
> +    FEAT(dual_ppr_log_sup,   "Dual PPR Log");
> +    FEAT(dual_event_log_sup, "Dual Event Log");
> +    FEAT(sats_sup,           "Secure ATS");
> +    FEAT(us_sup,             "User / Supervisor Page Protection");
> +    FEAT(dev_tbl_seg_sup,    "Device Table Segmentation");
> +    FEAT(ppr_early_of_sup,   "PPR Log Overflow Early Warning");
> +    FEAT(ppr_auto_rsp_sup,   "PPR Automatic Response");
> +    FEAT(marc_sup,           "Memory Access Routing and Control");
> +    FEAT(blk_stop_mrk_sup,   "Block StopMark Message");
> +    FEAT(perf_opt_sup ,      "Performance Optimization");
> +    FEAT(msi_cap_mmio_sup,   "MSI Capability MMIO Access");
> +    FEAT(gio_sup,            "Guest I/O Protection");
> +    FEAT(ha_sup,             "Host Access");
> +    FEAT(eph_sup,            "Enhanced PPR Handling");
> +    FEAT(attr_fw_sup,        "Attribute Forward");
> +    FEAT(hd_sup,             "Host Dirty");
> +    FEAT(inv_iotlb_type_sup, "Invalidate IOTLB Type");
> +    FEAT(viommu_sup,         "Virtualized IOMMU");
> +    FEAT(vm_guard_io_sup,    "VMGuard I/O Support");
> +    FEAT(vm_table_size,      "VM Table Size");
> +    FEAT(ga_update_dis_sup,  "Guest Access Bit Update Disable");
> +
> +#undef FEAT
>   }
>   
>   int __init amd_iommu_detect_one_acpi(
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -638,7 +638,7 @@ static uint64_t iommu_mmio_read64(struct
>           val = reg_to_u64(iommu->reg_status);
>           break;
>       case IOMMU_EXT_FEATURE_MMIO_OFFSET:
> -        val = reg_to_u64(iommu->reg_ext_feature);
> +        val = iommu->reg_ext_feature.raw;
>           break;
>   
>       default:
> @@ -802,39 +802,26 @@ int guest_iommu_set_base(struct domain *
>   /* Initialize mmio read only bits */
>   static void guest_iommu_reg_init(struct guest_iommu *iommu)
>   {
> -    uint32_t lower, upper;
> +    union amd_iommu_ext_features ef = {
> +        /* Support prefetch */
> +        .flds.pref_sup = 1,
> +        /* Support PPR log */
> +        .flds.ppr_sup = 1,
> +        /* Support guest translation */
> +        .flds.gt_sup = 1,
> +        /* Support invalidate all command */
> +        .flds.ia_sup = 1,
> +        /* Host translation size has 6 levels */
> +        .flds.hats = HOST_ADDRESS_SIZE_6_LEVEL,
> +        /* Guest translation size has 6 levels */
> +        .flds.gats = GUEST_ADDRESS_SIZE_6_LEVEL,
> +        /* Single level gCR3 */
> +        .flds.glx_sup = GUEST_CR3_1_LEVEL,
> +        /* 9 bit PASID */
> +        .flds.pas_max = PASMAX_9_bit,
> +    };
>   
> -    lower = upper = 0;
> -    /* Support prefetch */
> -    iommu_set_bit(&lower,IOMMU_EXT_FEATURE_PREFSUP_SHIFT);
> -    /* Support PPR log */
> -    iommu_set_bit(&lower,IOMMU_EXT_FEATURE_PPRSUP_SHIFT);
> -    /* Support guest translation */
> -    iommu_set_bit(&lower,IOMMU_EXT_FEATURE_GTSUP_SHIFT);
> -    /* Support invalidate all command */
> -    iommu_set_bit(&lower,IOMMU_EXT_FEATURE_IASUP_SHIFT);
> -
> -    /* Host translation size has 6 levels */
> -    set_field_in_reg_u32(HOST_ADDRESS_SIZE_6_LEVEL, lower,
> -                         IOMMU_EXT_FEATURE_HATS_MASK,
> -                         IOMMU_EXT_FEATURE_HATS_SHIFT,
> -                         &lower);
> -    /* Guest translation size has 6 levels */
> -    set_field_in_reg_u32(GUEST_ADDRESS_SIZE_6_LEVEL, lower,
> -                         IOMMU_EXT_FEATURE_GATS_MASK,
> -                         IOMMU_EXT_FEATURE_GATS_SHIFT,
> -                         &lower);
> -    /* Single level gCR3 */
> -    set_field_in_reg_u32(GUEST_CR3_1_LEVEL, lower,
> -                         IOMMU_EXT_FEATURE_GLXSUP_MASK,
> -                         IOMMU_EXT_FEATURE_GLXSUP_SHIFT, &lower);
> -    /* 9 bit PASID */
> -    set_field_in_reg_u32(PASMAX_9_bit, upper,
> -                         IOMMU_EXT_FEATURE_PASMAX_MASK,
> -                         IOMMU_EXT_FEATURE_PASMAX_SHIFT, &upper);
> -
> -    iommu->reg_ext_feature.lo = lower;
> -    iommu->reg_ext_feature.hi = upper;
> +    iommu->reg_ext_feature = ef;
>   }
>   
>   static int guest_iommu_mmio_range(struct vcpu *v, unsigned long addr)
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -882,7 +882,7 @@ static void enable_iommu(struct amd_iomm
>       register_iommu_event_log_in_mmio_space(iommu);
>       register_iommu_exclusion_range(iommu);
>   
> -    if ( amd_iommu_has_feature(iommu, IOMMU_EXT_FEATURE_PPRSUP_SHIFT) )
> +    if ( iommu->features.flds.ppr_sup )
>           register_iommu_ppr_log_in_mmio_space(iommu);
>   
>       desc = irq_to_desc(iommu->msi.irq);
> @@ -896,15 +896,15 @@ static void enable_iommu(struct amd_iomm
>       set_iommu_command_buffer_control(iommu, IOMMU_CONTROL_ENABLED);
>       set_iommu_event_log_control(iommu, IOMMU_CONTROL_ENABLED);
>   
> -    if ( amd_iommu_has_feature(iommu, IOMMU_EXT_FEATURE_PPRSUP_SHIFT) )
> +    if ( iommu->features.flds.ppr_sup )
>           set_iommu_ppr_log_control(iommu, IOMMU_CONTROL_ENABLED);
>   
> -    if ( amd_iommu_has_feature(iommu, IOMMU_EXT_FEATURE_GTSUP_SHIFT) )
> +    if ( iommu->features.flds.gt_sup )
>           set_iommu_guest_translation_control(iommu, IOMMU_CONTROL_ENABLED);
>   
>       set_iommu_translation_control(iommu, IOMMU_CONTROL_ENABLED);
>   
> -    if ( amd_iommu_has_feature(iommu, IOMMU_EXT_FEATURE_IASUP_SHIFT) )
> +    if ( iommu->features.flds.ia_sup )
>           amd_iommu_flush_all_caches(iommu);
>   
>       iommu->enabled = 1;
> @@ -927,10 +927,10 @@ static void disable_iommu(struct amd_iom
>       set_iommu_command_buffer_control(iommu, IOMMU_CONTROL_DISABLED);
>       set_iommu_event_log_control(iommu, IOMMU_CONTROL_DISABLED);
>   
> -    if ( amd_iommu_has_feature(iommu, IOMMU_EXT_FEATURE_PPRSUP_SHIFT) )
> +    if ( iommu->features.flds.ppr_sup )
>           set_iommu_ppr_log_control(iommu, IOMMU_CONTROL_DISABLED);
>   
> -    if ( amd_iommu_has_feature(iommu, IOMMU_EXT_FEATURE_GTSUP_SHIFT) )
> +    if ( iommu->features.flds.gt_sup )
>           set_iommu_guest_translation_control(iommu, IOMMU_CONTROL_DISABLED);
>   
>       set_iommu_translation_control(iommu, IOMMU_CONTROL_DISABLED);
> @@ -1026,7 +1026,7 @@ static int __init amd_iommu_init_one(str
>   
>       get_iommu_features(iommu);
>   
> -    if ( iommu->features )
> +    if ( iommu->features.raw )
>           iommuv2_enabled = 1;
>   
>       if ( allocate_cmd_buffer(iommu) == NULL )
> @@ -1035,9 +1035,8 @@ static int __init amd_iommu_init_one(str
>       if ( allocate_event_log(iommu) == NULL )
>           goto error_out;
>   
> -    if ( amd_iommu_has_feature(iommu, IOMMU_EXT_FEATURE_PPRSUP_SHIFT) )
> -        if ( allocate_ppr_log(iommu) == NULL )
> -            goto error_out;
> +    if ( iommu->features.flds.ppr_sup && !allocate_ppr_log(iommu) )
> +        goto error_out;
>   
>       if ( !set_iommu_interrupt_handler(iommu) )
>           goto error_out;
> @@ -1393,7 +1392,7 @@ void amd_iommu_resume(void)
>       }
>   
>       /* flush all cache entries after iommu re-enabled */
> -    if ( !amd_iommu_has_feature(iommu, IOMMU_EXT_FEATURE_IASUP_SHIFT) )
> +    if ( !iommu->features.flds.ia_sup )
>       {
>           invalidate_all_devices();
>           invalidate_all_domain_pages();
> --- a/xen/include/asm-x86/amd-iommu.h
> +++ b/xen/include/asm-x86/amd-iommu.h
> @@ -83,7 +83,7 @@ struct amd_iommu {
>       iommu_cap_t cap;
>   
>       u8 ht_flags;
> -    u64 features;
> +    union amd_iommu_ext_features features;
>   
>       void *mmio_base;
>       unsigned long mmio_base_phys;
> @@ -175,7 +175,7 @@ struct guest_iommu {
>       /* MMIO regs */
>       struct mmio_reg         reg_ctrl;              /* MMIO offset 0018h */
>       struct mmio_reg         reg_status;            /* MMIO offset 2020h */
> -    struct mmio_reg         reg_ext_feature;       /* MMIO offset 0030h */
> +    union amd_iommu_ext_features reg_ext_feature;  /* MMIO offset 0030h */
>   
>       /* guest interrupt settings */
>       struct guest_iommu_msi  msi;
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -346,26 +346,57 @@ struct amd_iommu_dte {
>   #define IOMMU_EXCLUSION_LIMIT_HIGH_MASK             0xFFFFFFFF
>   #define IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT    0
>   
> -/* Extended Feature Register*/
> +/* Extended Feature Register */
>   #define IOMMU_EXT_FEATURE_MMIO_OFFSET                   0x30
> -#define IOMMU_EXT_FEATURE_PREFSUP_SHIFT                 0x0
> -#define IOMMU_EXT_FEATURE_PPRSUP_SHIFT                  0x1
> -#define IOMMU_EXT_FEATURE_XTSUP_SHIFT                   0x2
> -#define IOMMU_EXT_FEATURE_NXSUP_SHIFT                   0x3
> -#define IOMMU_EXT_FEATURE_GTSUP_SHIFT                   0x4
> -#define IOMMU_EXT_FEATURE_IASUP_SHIFT                   0x6
> -#define IOMMU_EXT_FEATURE_GASUP_SHIFT                   0x7
> -#define IOMMU_EXT_FEATURE_HESUP_SHIFT                   0x8
> -#define IOMMU_EXT_FEATURE_PCSUP_SHIFT                   0x9
> -#define IOMMU_EXT_FEATURE_HATS_SHIFT                    0x10
> -#define IOMMU_EXT_FEATURE_HATS_MASK                     0x00000C00
> -#define IOMMU_EXT_FEATURE_GATS_SHIFT                    0x12
> -#define IOMMU_EXT_FEATURE_GATS_MASK                     0x00003000
> -#define IOMMU_EXT_FEATURE_GLXSUP_SHIFT                  0x14
> -#define IOMMU_EXT_FEATURE_GLXSUP_MASK                   0x0000C000
>   
> -#define IOMMU_EXT_FEATURE_PASMAX_SHIFT                  0x0
> -#define IOMMU_EXT_FEATURE_PASMAX_MASK                   0x0000001F
> +union amd_iommu_ext_features {
> +    uint64_t raw;
> +    struct {
> +        unsigned int pref_sup:1;
> +        unsigned int ppr_sup:1;
> +        unsigned int xt_sup:1;
> +        unsigned int nx_sup:1;
> +        unsigned int gt_sup:1;
> +        unsigned int gappi_sup:1;
> +        unsigned int ia_sup:1;
> +        unsigned int ga_sup:1;
> +        unsigned int he_sup:1;
> +        unsigned int pc_sup:1;
> +        unsigned int hats:2;
> +        unsigned int gats:2;
> +        unsigned int glx_sup:2;
> +        unsigned int smif_sup:2;
> +        unsigned int smif_rc:3;
> +        unsigned int gam_sup:3;
> +        unsigned int dual_ppr_log_sup:2;
> +        unsigned int :2;
> +        unsigned int dual_event_log_sup:2;
> +        unsigned int :1;
> +        unsigned int sats_sup:1;
> +        unsigned int pas_max:5;
> +        unsigned int us_sup:1;
> +        unsigned int dev_tbl_seg_sup:2;
> +        unsigned int ppr_early_of_sup:1;
> +        unsigned int ppr_auto_rsp_sup:1;
> +        unsigned int marc_sup:2;
> +        unsigned int blk_stop_mrk_sup:1;
> +        unsigned int perf_opt_sup:1;
> +        unsigned int msi_cap_mmio_sup:1;
> +        unsigned int :1;
> +        unsigned int gio_sup:1;
> +        unsigned int ha_sup:1;
> +        unsigned int eph_sup:1;
> +        unsigned int attr_fw_sup:1;
> +        unsigned int hd_sup:1;
> +        unsigned int :1;
> +        unsigned int inv_iotlb_type_sup:1;
> +        unsigned int viommu_sup:1;
> +        unsigned int vm_guard_io_sup:1;
> +        unsigned int vm_table_size:4;
> +        unsigned int ga_update_dis_sup:1;
> +        unsigned int :2;
> +    } flds;
> +};
>   
>   /* Status Register*/
>   #define IOMMU_STATUS_MMIO_OFFSET            0x2020
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> @@ -218,13 +218,6 @@ static inline int iommu_has_cap(struct a
>       return !!(iommu->cap.header & (1u << bit));
>   }
>   
> -static inline int amd_iommu_has_feature(struct amd_iommu *iommu, uint32_t 
> bit)
> -{
> -    if ( !iommu_has_cap(iommu, PCI_CAP_EFRSUP_SHIFT) )
> -        return 0;
> -    return !!(iommu->features & (1U << bit));
> -}
> -
>   /* access tail or head pointer of ring buffer */
>   static inline uint32_t iommu_get_rb_pointer(uint32_t reg)
>   {
> 

-- 
Brian Woods

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.