On 27/06/2019 16:21, Jan Beulich wrote:
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -40,12 +40,45 @@ union irte32 {
>
> -#define INTREMAP_TABLE_ORDER 1
> +union irte_cptr {
> + const void *ptr;
> + const union irte32 *ptr32;
> + const union irte128 *ptr128;
> +} __transparent__;
> +
> +#define INTREMAP_TABLE_ORDER (irte_mode == irte32 ? 1 : 3)
This is problematic for irte_mode == irteUNK. As this "constant" is
used in exactly two places, I'd suggest a tiny static function along the
same lines as {get,update}_intremap_entry(), which can sensibly prevent
code looking for a size before irte_mode is set up.
> @@ -142,7 +187,21 @@ static void free_intremap_entry(unsigned
> {
> union irte_ptr entry = get_intremap_entry(seg, bdf, index);
>
> - ACCESS_ONCE(entry.ptr32->raw[0]) = 0;
> + switch ( irte_mode )
> + {
> + case irte32:
> + ACCESS_ONCE(entry.ptr32->raw[0]) = 0;
> + break;
> +
> + case irte128:
> + ACCESS_ONCE(entry.ptr128->raw[0]) = 0;
> + barrier();
smp_wmb().
Using barrier here isn't technically correct, because what matters is
the external visibility of the write.
It functions correctly on x86 because smp_wmb() is barrier(), but this
code doesn't work correctly on e.g. ARM.
I'd go further and leave an explanation.
smp_wmb(); /* Ensure the clear of .remap_en is visible to the IOMMU
first. */
> @@ -444,9 +601,9 @@ static int update_intremap_entry_from_ms
> unsigned long flags;
> union irte_ptr entry;
> u16 req_id, alias_id;
> - u8 delivery_mode, dest, vector, dest_mode;
> + uint8_t delivery_mode, vector, dest_mode;
For the ioapic version, you used unsigned int, rather than uint8_t. I'd
expect them to at least be consistent.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
