On 19/07/2019 17:16, Jan Beulich wrote:
> On 19.07.2019 17:56, Andrew Cooper wrote:
>> On 16/07/2019 17:36, 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 <jbeul...@suse.com>
>> I'm still not entirely convinced by extra union and containerof(), but
>> the result looks correct.
> And I'm still open to going the other way, if you're convinced that
> in update_intremap_entry() this
>
> struct irte_basic basic = {
> .flds = {
> .remap_en = true,
> .int_type = int_type,
> .dm = dest_mode,
> .dest = dest,
> .vector = vector,
> }
> };
>
> (and similarly then for the 128-bit form, and of course .flds
> inserted at other use sites) is overall better than the current
> variant.
I've just experimented with the attached delta and it does compile in
CentOS 6, which is the usual culprit for problems in this area.
I do think the result is easier-to-read code, which I am definitely in
favour of.
My ack still stands in all affected patches, but ideally with this kind
of change folded in appropriately.
~Andrew
>From e028cb655e889ca392023925280f30729be29be7 Mon Sep 17 00:00:00 2001
From: Andrew Cooper <andrew.coop...@citrix.com>
Date: Fri, 19 Jul 2019 19:20:27 +0100
Subject: Experiment without containerof
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index d8d0f71d0d..dc781bb9f1 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -24,41 +24,37 @@
#include <xen/keyhandler.h>
#include <xen/softirq.h>
-struct irte_basic {
- bool remap_en:1;
- bool sup_io_pf:1;
- unsigned int int_type:3;
- bool rq_eoi:1;
- bool dm:1;
- bool guest_mode:1; /* MBZ */
- unsigned int dest:8;
- unsigned int vector:8;
- unsigned int :8;
-};
-
union irte32 {
- uint32_t raw[1];
- struct irte_basic basic;
-};
-
-struct irte_full {
- bool remap_en:1;
- bool sup_io_pf:1;
- unsigned int int_type:3;
- bool rq_eoi:1;
- bool dm:1;
- bool guest_mode:1; /* MBZ */
- unsigned int dest_lo:24;
- unsigned int :32;
- unsigned int vector:8;
- unsigned int :24;
- unsigned int :24;
- unsigned int dest_hi:8;
+ uint32_t raw;
+ struct {
+ bool remap_en:1;
+ bool sup_io_pf:1;
+ unsigned int int_type:3;
+ bool rq_eoi:1;
+ bool dm:1;
+ bool guest_mode:1; /* MBZ */
+ unsigned int dest:8;
+ unsigned int vector:8;
+ unsigned int :8;
+ } flds;
};
union irte128 {
uint64_t raw[2];
- struct irte_full full;
+ struct {
+ bool remap_en:1;
+ bool sup_io_pf:1;
+ unsigned int int_type:3;
+ bool rq_eoi:1;
+ bool dm:1;
+ bool guest_mode:1; /* MBZ */
+ unsigned int dest_lo:24;
+ unsigned int :32;
+ unsigned int vector:8;
+ unsigned int :24;
+ unsigned int :24;
+ unsigned int dest_hi:8;
+ } flds;
};
union irte_ptr {
@@ -187,7 +183,7 @@ static void free_intremap_entry(const struct amd_iommu *iommu,
entry.ptr128->raw[1] = 0;
}
else
- ACCESS_ONCE(entry.ptr32->raw[0]) = 0;
+ ACCESS_ONCE(entry.ptr32->raw) = 0;
__clear_bit(index, get_ivrs_mappings(iommu->seg)[bdf].intremap_inuse);
}
@@ -199,35 +195,36 @@ static void update_intremap_entry(const struct amd_iommu *iommu,
{
if ( iommu->ctrl.ga_en )
{
- struct irte_full full = {
- .remap_en = true,
- .int_type = int_type,
- .dm = dest_mode,
- .dest_lo = dest,
- .dest_hi = dest >> 24,
- .vector = vector,
+ union irte128 irte = {
+ .flds = {
+ .remap_en = true,
+ .int_type = int_type,
+ .dm = dest_mode,
+ .dest_lo = dest,
+ .dest_hi = dest >> 24,
+ .vector = vector,
+ },
};
- ASSERT(!entry.ptr128->full.remap_en);
- entry.ptr128->raw[1] =
- container_of(&full, union irte128, full)->raw[1];
+ ASSERT(!entry.ptr128->flds.remap_en);
+ entry.ptr128->raw[1] = irte.raw[1];
/* High half needs to be set before low one (containing RemapEn). */
smp_wmb();
- ACCESS_ONCE(entry.ptr128->raw[0]) =
- container_of(&full, union irte128, full)->raw[0];
+ ACCESS_ONCE(entry.ptr128->raw[0]) = irte.raw[0];
}
else
{
- struct irte_basic basic = {
- .remap_en = true,
- .int_type = int_type,
- .dm = dest_mode,
- .dest = dest,
- .vector = vector,
+ union irte32 irte = {
+ .flds = {
+ .remap_en = true,
+ .int_type = int_type,
+ .dm = dest_mode,
+ .dest = dest,
+ .vector = vector,
+ },
};
- ACCESS_ONCE(entry.ptr32->raw[0]) =
- container_of(&basic, union irte32, basic)->raw[0];
+ ACCESS_ONCE(entry.ptr32->raw) = irte.raw;
}
}
@@ -244,7 +241,7 @@ static inline void set_rte_index(struct IO_APIC_route_entry *rte, int offset)
static inline unsigned int get_full_dest(const union irte128 *entry)
{
- return entry->full.dest_lo | ((unsigned int)entry->full.dest_hi << 24);
+ return entry->flds.dest_lo | ((unsigned int)entry->flds.dest_hi << 24);
}
static int update_intremap_entry_from_ioapic(
@@ -289,9 +286,9 @@ static int update_intremap_entry_from_ioapic(
entry = get_intremap_entry(iommu, req_id, offset);
/* The RemapEn fields match for all formats. */
- while ( iommu->enabled && entry.ptr32->basic.remap_en )
+ while ( iommu->enabled && entry.ptr32->flds.remap_en )
{
- entry.ptr32->basic.remap_en = false;
+ entry.ptr32->flds.remap_en = false;
spin_unlock(lock);
spin_lock(&iommu->lock);
@@ -311,11 +308,11 @@ static int update_intremap_entry_from_ioapic(
*/
ASSERT(get_rte_index(rte) == offset);
if ( iommu->ctrl.ga_en )
- vector = entry.ptr128->full.vector;
+ vector = entry.ptr128->flds.vector;
else
- vector = entry.ptr32->basic.vector;
+ vector = entry.ptr32->flds.vector;
/* The IntType fields match for both formats. */
- delivery_mode = entry.ptr32->basic.int_type;
+ delivery_mode = entry.ptr32->flds.int_type;
}
else if ( x2apic_enabled )
{
@@ -558,11 +555,11 @@ unsigned int amd_iommu_read_ioapic_from_ire(
ASSERT(offset == (val & (INTREMAP_ENTRIES - 1)));
val &= ~(INTREMAP_ENTRIES - 1);
/* The IntType fields match for both formats. */
- val |= MASK_INSR(entry.ptr32->basic.int_type,
+ val |= MASK_INSR(entry.ptr32->flds.int_type,
IO_APIC_REDIR_DELIV_MODE_MASK);
val |= MASK_INSR(iommu->ctrl.ga_en
- ? entry.ptr128->full.vector
- : entry.ptr32->basic.vector,
+ ? entry.ptr128->flds.vector
+ : entry.ptr32->flds.vector,
IO_APIC_REDIR_VECTOR_MASK);
}
else if ( x2apic_enabled )
@@ -631,9 +628,9 @@ static int update_intremap_entry_from_msi_msg(
entry = get_intremap_entry(iommu, req_id, offset);
/* The RemapEn fields match for all formats. */
- while ( iommu->enabled && entry.ptr32->basic.remap_en )
+ while ( iommu->enabled && entry.ptr32->flds.remap_en )
{
- entry.ptr32->basic.remap_en = false;
+ entry.ptr32->flds.remap_en = false;
spin_unlock(lock);
spin_lock(&iommu->lock);
@@ -767,19 +764,19 @@ void amd_iommu_read_msi_from_ire(
msg->data &= ~(INTREMAP_ENTRIES - 1);
/* The IntType fields match for both formats. */
- msg->data |= MASK_INSR(entry.ptr32->basic.int_type,
+ msg->data |= MASK_INSR(entry.ptr32->flds.int_type,
MSI_DATA_DELIVERY_MODE_MASK);
if ( iommu->ctrl.ga_en )
{
- msg->data |= MASK_INSR(entry.ptr128->full.vector,
+ msg->data |= MASK_INSR(entry.ptr128->flds.vector,
MSI_DATA_VECTOR_MASK);
msg->dest32 = get_full_dest(entry.ptr128);
}
else
{
- msg->data |= MASK_INSR(entry.ptr32->basic.vector,
+ msg->data |= MASK_INSR(entry.ptr32->flds.vector,
MSI_DATA_VECTOR_MASK);
- msg->dest32 = entry.ptr32->basic.dest;
+ msg->dest32 = entry.ptr32->flds.dest;
}
}
@@ -894,9 +891,9 @@ static void dump_intremap_table(const struct amd_iommu *iommu,
}
else
{
- if ( !tbl.ptr32[count].raw[0] )
+ if ( !tbl.ptr32[count].raw )
continue;
- printk(" IRTE[%03x] %08x\n", count, tbl.ptr32[count].raw[0]);
+ printk(" IRTE[%03x] %08x\n", count, tbl.ptr32[count].raw);
}
}
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
