diff mbox series

[3/9] AMD/IOMMU: use bit field for IRTE

Message ID 5D024E3E0200007800237E03@prv1-mh.provo.novell.com (mailing list archive)
State Superseded
Headers show
Series x86: AMD x2APIC support | expand

Commit Message

Jan Beulich June 13, 2019, 1:23 p.m. UTC
At the same time restrict its scope to just the single source file
actually using it, and abstract accesses by introducing a union of
pointers. (A union of the actual table entries is not used to make it
impossible to [wrongly, once the 128-bit form gets added] perform
pointer arithmetic / array accesses on derived types.)

Also move away from updating the entries piecemeal: Construct a full new
entry, and write it out.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
It would have been nice to use write_atomic() or ACCESS_ONCE() for the
actual writes, but both cast the value to a scalar one, which doesn't
suit us here (and I also didn't want to make the compound type a union
with a raw member just for this).

Comments

Andrew Cooper June 18, 2019, 10:37 a.m. UTC | #1
On 13/06/2019 14:23, Jan Beulich wrote:
> At the same time restrict its scope to just the single source file
> actually using it, and abstract accesses by introducing a union of
> pointers. (A union of the actual table entries is not used to make it
> impossible to [wrongly, once the 128-bit form gets added] perform
> pointer arithmetic / array accesses on derived types.)
>
> Also move away from updating the entries piecemeal: Construct a full new
> entry, and write it out.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> It would have been nice to use write_atomic() or ACCESS_ONCE() for the
> actual writes, but both cast the value to a scalar one, which doesn't
> suit us here (and I also didn't want to make the compound type a union
> with a raw member just for this).
>
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -23,6 +23,23 @@
>  #include <asm/io_apic.h>
>  #include <xen/keyhandler.h>
>  
> +struct irte_basic {

I'd suggest irte_32, to go with irte_128 in the following patch. 

The 128bit format is also used for posted interrupts, and isn't specific
to x2apic support.

Furthermore, calling it irte_full isn't a term I can see in the manual,
and is falling into the naming trap that USB currently lives in.

> +    unsigned int remap_en:1;
> +    unsigned int sup_io_pf:1;
> +    unsigned int int_type:3;
> +    unsigned int rq_eoi:1;
> +    unsigned int dm:1;
> +    unsigned int guest_mode:1; /* MBZ */
> +    unsigned int dest:8;
> +    unsigned int vector:8;
> +    unsigned int :8;
> +};
> +
> +union irte_ptr {
> +    void *raw;
> +    struct irte_basic *basic;
> +};
> +
>  #define INTREMAP_TABLE_ORDER    1
>  #define INTREMAP_LENGTH 0xB
>  #define INTREMAP_ENTRIES (1 << INTREMAP_LENGTH)
> @@ -101,47 +118,44 @@ static unsigned int alloc_intremap_entry
>      return slot;
>  }
>  
> -static u32 *get_intremap_entry(int seg, int bdf, int offset)
> +static union irte_ptr get_intremap_entry(unsigned int seg, unsigned int bdf,
> +                                         unsigned int offset)

As this is changing, s/offset/entry/ to avoid any confusion where offset
might be in units of bytes.

~Andrew
Andrew Cooper June 18, 2019, 11:31 a.m. UTC | #2
On 13/06/2019 14:23, Jan Beulich wrote:
> At the same time restrict its scope to just the single source file
> actually using it, and abstract accesses by introducing a union of
> pointers. (A union of the actual table entries is not used to make it
> impossible to [wrongly, once the 128-bit form gets added] perform
> pointer arithmetic / array accesses on derived types.)
>
> Also move away from updating the entries piecemeal: Construct a full new
> entry, and write it out.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> It would have been nice to use write_atomic() or ACCESS_ONCE() for the
> actual writes, but both cast the value to a scalar one, which doesn't
> suit us here (and I also didn't want to make the compound type a union
> with a raw member just for this).

Actually, having looked at the following patch, I think it would be
better to union with a uint32_t raw, so that we can use...

> @@ -101,47 +118,44 @@ static unsigned int alloc_intremap_entry
>      return slot;
>  }
>  
> -static u32 *get_intremap_entry(int seg, int bdf, int offset)
> +static union irte_ptr get_intremap_entry(unsigned int seg, unsigned int bdf,
> +                                         unsigned int offset)
>  {
> -    u32 *table = get_ivrs_mappings(seg)[bdf].intremap_table;
> +    union irte_ptr table = {
> +        .raw = get_ivrs_mappings(seg)[bdf].intremap_table
> +    };
> +
> +    ASSERT(table.raw && (offset < INTREMAP_ENTRIES));
>  
> -    ASSERT( (table != NULL) && (offset < INTREMAP_ENTRIES) );
> +    table.basic += offset;
>  
> -    return table + offset;
> +    return table;
>  }
>  
> -static void free_intremap_entry(int seg, int bdf, int offset)
> +static void free_intremap_entry(unsigned int seg, unsigned int bdf, unsigned int offset)
>  {
> -    u32 *entry = get_intremap_entry(seg, bdf, offset);
> +    union irte_ptr entry = get_intremap_entry(seg, bdf, offset);
> +
> +    *entry.basic = (struct irte_basic){};

ACCESS_ONCE(*entry.basic.raw) = 0;

and...

>  
> -    memset(entry, 0, sizeof(u32));
>      __clear_bit(offset, get_ivrs_mappings(seg)[bdf].intremap_inuse);
>  }
>  
> -static void update_intremap_entry(u32* entry, u8 vector, u8 int_type,
> -    u8 dest_mode, u8 dest)
> -{
> -    set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, 0,
> -                            INT_REMAP_ENTRY_REMAPEN_MASK,
> -                            INT_REMAP_ENTRY_REMAPEN_SHIFT, entry);
> -    set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, *entry,
> -                            INT_REMAP_ENTRY_SUPIOPF_MASK,
> -                            INT_REMAP_ENTRY_SUPIOPF_SHIFT, entry);
> -    set_field_in_reg_u32(int_type, *entry,
> -                            INT_REMAP_ENTRY_INTTYPE_MASK,
> -                            INT_REMAP_ENTRY_INTTYPE_SHIFT, entry);
> -    set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, *entry,
> -                            INT_REMAP_ENTRY_REQEOI_MASK,
> -                            INT_REMAP_ENTRY_REQEOI_SHIFT, entry);
> -    set_field_in_reg_u32((u32)dest_mode, *entry,
> -                            INT_REMAP_ENTRY_DM_MASK,
> -                            INT_REMAP_ENTRY_DM_SHIFT, entry);
> -    set_field_in_reg_u32((u32)dest, *entry,
> -                            INT_REMAP_ENTRY_DEST_MAST,
> -                            INT_REMAP_ENTRY_DEST_SHIFT, entry);
> -    set_field_in_reg_u32((u32)vector, *entry,
> -                            INT_REMAP_ENTRY_VECTOR_MASK,
> -                            INT_REMAP_ENTRY_VECTOR_SHIFT, entry);
> +static void update_intremap_entry(union irte_ptr entry, unsigned int vector,
> +                                  unsigned int int_type,
> +                                  unsigned int dest_mode, unsigned int dest)
> +{
> +    struct irte_basic basic = {
> +        .remap_en = 1,
> +        .sup_io_pf = 0,
> +        .int_type = int_type,
> +        .rq_eoi = 0,
> +        .dm = dest_mode,
> +        .dest = dest,
> +        .vector = vector,
> +    };
> +
> +    *entry.basic = basic;

ACCESS_ONCE(*entry.basic.raw) = basic.raw.

The problem is in an unoptimised case, structure assignment in
implemented with memcpy(), which may be implemented as `rep stosb` which
may result in a spliced write with the enable bit set first.

Using a union with raw allows for the use of ACCESS_ONCE(), which forces
the compiler to implement them as 32bit single mov's.

~Andrew
Jan Beulich June 18, 2019, 11:47 a.m. UTC | #3
>>> On 18.06.19 at 13:31, <andrew.cooper3@citrix.com> wrote:
> On 13/06/2019 14:23, Jan Beulich wrote:
>> At the same time restrict its scope to just the single source file
>> actually using it, and abstract accesses by introducing a union of
>> pointers. (A union of the actual table entries is not used to make it
>> impossible to [wrongly, once the 128-bit form gets added] perform
>> pointer arithmetic / array accesses on derived types.)
>>
>> Also move away from updating the entries piecemeal: Construct a full new
>> entry, and write it out.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> It would have been nice to use write_atomic() or ACCESS_ONCE() for the
>> actual writes, but both cast the value to a scalar one, which doesn't
>> suit us here (and I also didn't want to make the compound type a union
>> with a raw member just for this).
> 
> Actually, having looked at the following patch, I think it would be
> better to union with a uint32_t raw, so that we can use...

Well, I did in fact have it that way first, until ...

>> +static void update_intremap_entry(union irte_ptr entry, unsigned int vector,
>> +                                  unsigned int int_type,
>> +                                  unsigned int dest_mode, unsigned int dest)
>> +{
>> +    struct irte_basic basic = {
>> +        .remap_en = 1,
>> +        .sup_io_pf = 0,
>> +        .int_type = int_type,
>> +        .rq_eoi = 0,
>> +        .dm = dest_mode,
>> +        .dest = dest,
>> +        .vector = vector,
>> +    };

... I figured that this initializer then will require the bitfields part of
the union to also get named, like for union amd_iommu_ext_features
in patch 1.

>> +    *entry.basic = basic;
> 
> ACCESS_ONCE(*entry.basic.raw) = basic.raw.
> 
> The problem is in an unoptimised case, structure assignment in
> implemented with memcpy(), which may be implemented as `rep stosb` which
> may result in a spliced write with the enable bit set first.
> 
> Using a union with raw allows for the use of ACCESS_ONCE(), which forces
> the compiler to implement them as 32bit single mov's.

If we are worried about this, writing of 32-bit entries could be done
(as an alternative to what you suggest) just like that of 128-bit
ones by the last patch in the series.

Jan
Jan Beulich June 18, 2019, 11:53 a.m. UTC | #4
>>> On 18.06.19 at 12:37, <andrew.cooper3@citrix.com> wrote:
> On 13/06/2019 14:23, Jan Beulich wrote:
>> At the same time restrict its scope to just the single source file
>> actually using it, and abstract accesses by introducing a union of
>> pointers. (A union of the actual table entries is not used to make it
>> impossible to [wrongly, once the 128-bit form gets added] perform
>> pointer arithmetic / array accesses on derived types.)
>>
>> Also move away from updating the entries piecemeal: Construct a full new
>> entry, and write it out.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> It would have been nice to use write_atomic() or ACCESS_ONCE() for the
>> actual writes, but both cast the value to a scalar one, which doesn't
>> suit us here (and I also didn't want to make the compound type a union
>> with a raw member just for this).
>>
>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>> @@ -23,6 +23,23 @@
>>  #include <asm/io_apic.h>
>>  #include <xen/keyhandler.h>
>>  
>> +struct irte_basic {
> 
> I'd suggest irte_32, to go with irte_128 in the following patch. 
> 
> The 128bit format is also used for posted interrupts, and isn't specific
> to x2apic support.

There are still two forms of 128-bit entries, and the intention with
the chosen names was for the other one to become irte_guest.

> Furthermore, calling it irte_full isn't a term I can see in the manual,
> and is falling into the naming trap that USB currently lives in.

Except that other than for USB's transfer speeds I can't really see
this getting wider and wider.

>> @@ -101,47 +118,44 @@ static unsigned int alloc_intremap_entry
>>      return slot;
>>  }
>>  
>> -static u32 *get_intremap_entry(int seg, int bdf, int offset)
>> +static union irte_ptr get_intremap_entry(unsigned int seg, unsigned int bdf,
>> +                                         unsigned int offset)
> 
> As this is changing, s/offset/entry/ to avoid any confusion where offset
> might be in units of bytes.

I don't really mind - I think both names are sufficiently clear, but
I'll switch since you think the other name is better.

Jan
Andrew Cooper June 18, 2019, 12:16 p.m. UTC | #5
On 18/06/2019 12:53, Jan Beulich wrote:
>>>> On 18.06.19 at 12:37, <andrew.cooper3@citrix.com> wrote:
>> On 13/06/2019 14:23, Jan Beulich wrote:
>>> At the same time restrict its scope to just the single source file
>>> actually using it, and abstract accesses by introducing a union of
>>> pointers. (A union of the actual table entries is not used to make it
>>> impossible to [wrongly, once the 128-bit form gets added] perform
>>> pointer arithmetic / array accesses on derived types.)
>>>
>>> Also move away from updating the entries piecemeal: Construct a full new
>>> entry, and write it out.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> It would have been nice to use write_atomic() or ACCESS_ONCE() for the
>>> actual writes, but both cast the value to a scalar one, which doesn't
>>> suit us here (and I also didn't want to make the compound type a union
>>> with a raw member just for this).
>>>
>>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>>> @@ -23,6 +23,23 @@
>>>  #include <asm/io_apic.h>
>>>  #include <xen/keyhandler.h>
>>>  
>>> +struct irte_basic {
>> I'd suggest irte_32, to go with irte_128 in the following patch. 
>>
>> The 128bit format is also used for posted interrupts, and isn't specific
>> to x2apic support.
> There are still two forms of 128-bit entries, and the intention with
> the chosen names was for the other one to become irte_guest.

They are not forms of which can be delineated by irte_mode, because the
guest_mode setting is (/will be) per-domain, not global (which is
necessary for sane testability, and for nested-virt support where the
guest VMCB controls aren't set up by Xen).

>
>> Furthermore, calling it irte_full isn't a term I can see in the manual,
>> and is falling into the naming trap that USB currently lives in.
> Except that other than for USB's transfer speeds I can't really see
> this getting wider and wider.

It doesn't make the names "basic" and "full" any more descriptive.

>
>>> @@ -101,47 +118,44 @@ static unsigned int alloc_intremap_entry
>>>      return slot;
>>>  }
>>>  
>>> -static u32 *get_intremap_entry(int seg, int bdf, int offset)
>>> +static union irte_ptr get_intremap_entry(unsigned int seg, unsigned int bdf,
>>> +                                         unsigned int offset)
>> As this is changing, s/offset/entry/ to avoid any confusion where offset
>> might be in units of bytes.
> I don't really mind - I think both names are sufficiently clear, but
> I'll switch since you think the other name is better.

Looking through the other code, idx or index would also do fine, but I
think all of these are clearer than using offset.

~Andrew
Jan Beulich June 18, 2019, 12:55 p.m. UTC | #6
>>> On 18.06.19 at 14:16, <andrew.cooper3@citrix.com> wrote:
> On 18/06/2019 12:53, Jan Beulich wrote:
>>>>> On 18.06.19 at 12:37, <andrew.cooper3@citrix.com> wrote:
>>> On 13/06/2019 14:23, Jan Beulich wrote:
>>>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>>>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>>>> @@ -23,6 +23,23 @@
>>>>  #include <asm/io_apic.h>
>>>>  #include <xen/keyhandler.h>
>>>>  
>>>> +struct irte_basic {
>>> I'd suggest irte_32, to go with irte_128 in the following patch. 
>>>
>>> The 128bit format is also used for posted interrupts, and isn't specific
>>> to x2apic support.
>> There are still two forms of 128-bit entries, and the intention with
>> the chosen names was for the other one to become irte_guest.
> 
> They are not forms of which can be delineated by irte_mode, because the
> guest_mode setting is (/will be) per-domain, not global (which is
> necessary for sane testability, and for nested-virt support where the
> guest VMCB controls aren't set up by Xen).

True and ...

>>> Furthermore, calling it irte_full isn't a term I can see in the manual,
>>> and is falling into the naming trap that USB currently lives in.
>> Except that other than for USB's transfer speeds I can't really see
>> this getting wider and wider.
> 
> It doesn't make the names "basic" and "full" any more descriptive.

... also true, but irte_128 still won't fly with the other (guest) layout.

>>>> @@ -101,47 +118,44 @@ static unsigned int alloc_intremap_entry
>>>>      return slot;
>>>>  }
>>>>  
>>>> -static u32 *get_intremap_entry(int seg, int bdf, int offset)
>>>> +static union irte_ptr get_intremap_entry(unsigned int seg, unsigned int bdf,
>>>> +                                         unsigned int offset)
>>> As this is changing, s/offset/entry/ to avoid any confusion where offset
>>> might be in units of bytes.
>> I don't really mind - I think both names are sufficiently clear, but
>> I'll switch since you think the other name is better.
> 
> Looking through the other code, idx or index would also do fine, but I
> think all of these are clearer than using offset.

"index" it is then.

Jan
diff mbox series

Patch

--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -23,6 +23,23 @@ 
 #include <asm/io_apic.h>
 #include <xen/keyhandler.h>
 
+struct irte_basic {
+    unsigned int remap_en:1;
+    unsigned int sup_io_pf:1;
+    unsigned int int_type:3;
+    unsigned int rq_eoi:1;
+    unsigned int dm:1;
+    unsigned int guest_mode:1; /* MBZ */
+    unsigned int dest:8;
+    unsigned int vector:8;
+    unsigned int :8;
+};
+
+union irte_ptr {
+    void *raw;
+    struct irte_basic *basic;
+};
+
 #define INTREMAP_TABLE_ORDER    1
 #define INTREMAP_LENGTH 0xB
 #define INTREMAP_ENTRIES (1 << INTREMAP_LENGTH)
@@ -101,47 +118,44 @@  static unsigned int alloc_intremap_entry
     return slot;
 }
 
-static u32 *get_intremap_entry(int seg, int bdf, int offset)
+static union irte_ptr get_intremap_entry(unsigned int seg, unsigned int bdf,
+                                         unsigned int offset)
 {
-    u32 *table = get_ivrs_mappings(seg)[bdf].intremap_table;
+    union irte_ptr table = {
+        .raw = get_ivrs_mappings(seg)[bdf].intremap_table
+    };
+
+    ASSERT(table.raw && (offset < INTREMAP_ENTRIES));
 
-    ASSERT( (table != NULL) && (offset < INTREMAP_ENTRIES) );
+    table.basic += offset;
 
-    return table + offset;
+    return table;
 }
 
-static void free_intremap_entry(int seg, int bdf, int offset)
+static void free_intremap_entry(unsigned int seg, unsigned int bdf, unsigned int offset)
 {
-    u32 *entry = get_intremap_entry(seg, bdf, offset);
+    union irte_ptr entry = get_intremap_entry(seg, bdf, offset);
+
+    *entry.basic = (struct irte_basic){};
 
-    memset(entry, 0, sizeof(u32));
     __clear_bit(offset, get_ivrs_mappings(seg)[bdf].intremap_inuse);
 }
 
-static void update_intremap_entry(u32* entry, u8 vector, u8 int_type,
-    u8 dest_mode, u8 dest)
-{
-    set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, 0,
-                            INT_REMAP_ENTRY_REMAPEN_MASK,
-                            INT_REMAP_ENTRY_REMAPEN_SHIFT, entry);
-    set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, *entry,
-                            INT_REMAP_ENTRY_SUPIOPF_MASK,
-                            INT_REMAP_ENTRY_SUPIOPF_SHIFT, entry);
-    set_field_in_reg_u32(int_type, *entry,
-                            INT_REMAP_ENTRY_INTTYPE_MASK,
-                            INT_REMAP_ENTRY_INTTYPE_SHIFT, entry);
-    set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, *entry,
-                            INT_REMAP_ENTRY_REQEOI_MASK,
-                            INT_REMAP_ENTRY_REQEOI_SHIFT, entry);
-    set_field_in_reg_u32((u32)dest_mode, *entry,
-                            INT_REMAP_ENTRY_DM_MASK,
-                            INT_REMAP_ENTRY_DM_SHIFT, entry);
-    set_field_in_reg_u32((u32)dest, *entry,
-                            INT_REMAP_ENTRY_DEST_MAST,
-                            INT_REMAP_ENTRY_DEST_SHIFT, entry);
-    set_field_in_reg_u32((u32)vector, *entry,
-                            INT_REMAP_ENTRY_VECTOR_MASK,
-                            INT_REMAP_ENTRY_VECTOR_SHIFT, entry);
+static void update_intremap_entry(union irte_ptr entry, unsigned int vector,
+                                  unsigned int int_type,
+                                  unsigned int dest_mode, unsigned int dest)
+{
+    struct irte_basic basic = {
+        .remap_en = 1,
+        .sup_io_pf = 0,
+        .int_type = int_type,
+        .rq_eoi = 0,
+        .dm = dest_mode,
+        .dest = dest,
+        .vector = vector,
+    };
+
+    *entry.basic = basic;
 }
 
 static inline int get_rte_index(const struct IO_APIC_route_entry *rte)
@@ -163,7 +177,7 @@  static int update_intremap_entry_from_io
     u16 *index)
 {
     unsigned long flags;
-    u32* entry;
+    union irte_ptr entry;
     u8 delivery_mode, dest, vector, dest_mode;
     int req_id;
     spinlock_t *lock;
@@ -201,12 +215,8 @@  static int update_intremap_entry_from_io
          * so need to recover vector and delivery mode from IRTE.
          */
         ASSERT(get_rte_index(rte) == offset);
-        vector = get_field_from_reg_u32(*entry,
-                                        INT_REMAP_ENTRY_VECTOR_MASK,
-                                        INT_REMAP_ENTRY_VECTOR_SHIFT);
-        delivery_mode = get_field_from_reg_u32(*entry,
-                                               INT_REMAP_ENTRY_INTTYPE_MASK,
-                                               INT_REMAP_ENTRY_INTTYPE_SHIFT);
+        vector = entry.basic->vector;
+        delivery_mode = entry.basic->int_type;
     }
     update_intremap_entry(entry, vector, delivery_mode, dest_mode, dest);
 
@@ -228,7 +238,7 @@  int __init amd_iommu_setup_ioapic_remapp
 {
     struct IO_APIC_route_entry rte;
     unsigned long flags;
-    u32* entry;
+    union irte_ptr entry;
     int apic, pin;
     u8 delivery_mode, dest, vector, dest_mode;
     u16 seg, bdf, req_id;
@@ -407,16 +417,12 @@  unsigned int amd_iommu_read_ioapic_from_
         u16 bdf = ioapic_sbdf[idx].bdf;
         u16 seg = ioapic_sbdf[idx].seg;
         u16 req_id = get_intremap_requestor_id(seg, bdf);
-        const u32 *entry = get_intremap_entry(seg, req_id, offset);
+        union irte_ptr entry = get_intremap_entry(seg, req_id, offset);
 
         ASSERT(offset == (val & (INTREMAP_ENTRIES - 1)));
         val &= ~(INTREMAP_ENTRIES - 1);
-        val |= get_field_from_reg_u32(*entry,
-                                      INT_REMAP_ENTRY_INTTYPE_MASK,
-                                      INT_REMAP_ENTRY_INTTYPE_SHIFT) << 8;
-        val |= get_field_from_reg_u32(*entry,
-                                      INT_REMAP_ENTRY_VECTOR_MASK,
-                                      INT_REMAP_ENTRY_VECTOR_SHIFT);
+        val |= MASK_INSR(entry.basic->int_type, IO_APIC_REDIR_DELIV_MODE_MASK);
+        val |= MASK_INSR(entry.basic->vector, IO_APIC_REDIR_VECTOR_MASK);
     }
 
     return val;
@@ -427,7 +433,7 @@  static int update_intremap_entry_from_ms
     int *remap_index, const struct msi_msg *msg, u32 *data)
 {
     unsigned long flags;
-    u32* entry;
+    union irte_ptr entry;
     u16 req_id, alias_id;
     u8 delivery_mode, dest, vector, dest_mode;
     spinlock_t *lock;
@@ -581,7 +587,7 @@  void amd_iommu_read_msi_from_ire(
     const struct pci_dev *pdev = msi_desc->dev;
     u16 bdf = pdev ? PCI_BDF2(pdev->bus, pdev->devfn) : hpet_sbdf.bdf;
     u16 seg = pdev ? pdev->seg : hpet_sbdf.seg;
-    const u32 *entry;
+    union irte_ptr entry;
 
     if ( IS_ERR_OR_NULL(_find_iommu_for_device(seg, bdf)) )
         return;
@@ -597,12 +603,8 @@  void amd_iommu_read_msi_from_ire(
     }
 
     msg->data &= ~(INTREMAP_ENTRIES - 1);
-    msg->data |= get_field_from_reg_u32(*entry,
-                                        INT_REMAP_ENTRY_INTTYPE_MASK,
-                                        INT_REMAP_ENTRY_INTTYPE_SHIFT) << 8;
-    msg->data |= get_field_from_reg_u32(*entry,
-                                        INT_REMAP_ENTRY_VECTOR_MASK,
-                                        INT_REMAP_ENTRY_VECTOR_SHIFT);
+    msg->data |= MASK_INSR(entry.basic->int_type, MSI_DATA_DELIVERY_MODE_MASK);
+    msg->data |= MASK_INSR(entry.basic->vector, MSI_DATA_VECTOR_MASK);
 }
 
 int __init amd_iommu_free_intremap_table(
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -468,22 +468,6 @@  struct amd_iommu_pte {
 #define IOMMU_CONTROL_DISABLED	0
 #define IOMMU_CONTROL_ENABLED	1
 
-/* interrupt remapping table */
-#define INT_REMAP_ENTRY_REMAPEN_MASK    0x00000001
-#define INT_REMAP_ENTRY_REMAPEN_SHIFT   0
-#define INT_REMAP_ENTRY_SUPIOPF_MASK    0x00000002
-#define INT_REMAP_ENTRY_SUPIOPF_SHIFT   1
-#define INT_REMAP_ENTRY_INTTYPE_MASK    0x0000001C
-#define INT_REMAP_ENTRY_INTTYPE_SHIFT   2
-#define INT_REMAP_ENTRY_REQEOI_MASK     0x00000020
-#define INT_REMAP_ENTRY_REQEOI_SHIFT    5
-#define INT_REMAP_ENTRY_DM_MASK         0x00000040
-#define INT_REMAP_ENTRY_DM_SHIFT        6
-#define INT_REMAP_ENTRY_DEST_MAST       0x0000FF00
-#define INT_REMAP_ENTRY_DEST_SHIFT      8
-#define INT_REMAP_ENTRY_VECTOR_MASK     0x00FF0000
-#define INT_REMAP_ENTRY_VECTOR_SHIFT    16
-
 #define INV_IOMMU_ALL_PAGES_ADDRESS      ((1ULL << 63) - 1)
 
 #define IOMMU_RING_BUFFER_PTR_MASK                  0x0007FFF0