Re: [Xen-devel] [PATCH v3 1/4] iommu/amd: support all delivery modes with x2APIC

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2019 17:47, Roger Pau Monne wrote:
> Current AMD IOMMU code will attempt to create remapping entries for
> all IO-APIC pins, regardless of the delivery mode. AMD IOMMU
> implementation doesn't support remapping entries for IO-APIC pins with
> delivery mode set to SMI, MNI, INIT or ExtINT, instead those entries

Nit: "NMI"

> are not translated provided the right bits are set in the device table
> entry.
> 
> This change checks whether the device table entry has the correct bits
> set in order to delivery the requested interrupt or a warning message
> is printed. It also translates the 32bit destination field into a
> physical or logical IO-APIC entry format. Note that if the 32bit
> destination cannot fit into the legacy format a message is printed and
> the entry is masked.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

For this to have an effect on firmware initialized RTEs, it also
requires patch 4 aiui. In fact I think it should be _only_ this
case where we allow delivery modes other than fixed and lowest
priority ("arbitrated" in AMD terminology). Hence I think this
patch wants to go last in the series, and the code be changed to
reject runtime requests to fiddle with non-"normal" delivery
modes (this may go further and actually disallow changing such
RTEs at runtime alongside disallowing their production).

> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -439,6 +439,80 @@ int __init amd_iommu_setup_ioapic_remapping(void)
>      return 0;
>  }
>  
> +void setup_forced_ioapic_rte(unsigned int apic, unsigned int pin,
> +                             struct amd_iommu *iommu,
> +                             struct IO_APIC_route_entry *rte)
> +{
> +    unsigned int idx = ioapic_id_to_index(IO_APIC_ID(apic));
> +    struct amd_iommu_dte *table = iommu->dev_table.buffer;
> +    unsigned int req_id, dest, offset;
> +    union irte_ptr entry;
> +
> +    ASSERT(x2apic_enabled);
> +
> +    if ( idx == MAX_IO_APICS )

Better >= ?

> +    {
> +        rte->mask = true;
> +        return;
> +    }
> +
> +    req_id = get_intremap_requestor_id(ioapic_sbdf[idx].seg,
> +                                       ioapic_sbdf[idx].bdf);
> +
> +    switch ( rte->delivery_mode )
> +    {
> +    case dest_SMI:
> +        break;

Don't you want to check the sys_mgt field here, along the lines of the
other ones below?

> +#define DEL_CHECK(type, dte_field)                                      \
> +    case dest_ ## type:                                                 \
> +        if ( !table[req_id].dte_field )                                 \
> +            printk(XENLOG_WARNING                                       \
> +                   STR(type) " on IO-APIC %u pin %u will be aborted\n", \
> +                   apic, pin);                                          \
> +        break;

Please omit the final ; here, such that ...

> +    DEL_CHECK(NMI, nmi_pass);
> +    DEL_CHECK(INIT, init_pass);
> +    DEL_CHECK(ExtINT, ext_int_pass);

... the ones here become an actual requirement.

> +#undef DEL_CHECK
> +
> +    default:
> +        ASSERT_UNREACHABLE();
> +        return;
> +    }
> +
> +    offset = ioapic_sbdf[idx].pin_2_idx[pin];
> +    if ( offset >= INTREMAP_MAX_ENTRIES )
> +    {
> +        rte->mask = true;
> +        return;
> +    }
> +
> +    entry = get_intremap_entry(iommu, req_id, offset);
> +    dest = get_full_dest(entry.ptr128);
> +
> +#define SET_DEST(name, dest_mask) {                                          
>   \
> +    if ( dest & ~(dest_mask) )                                               
>   \
> +    {                                                                        
>   \
> +        printk(XENLOG_WARNING                                                
>   \
> +               "IO-APIC %u pin %u " STR(name) " destination (%x) > %x\n",    
>   \
> +               apic, pin, dest, dest_mask);                                  
>   \
> +        rte->mask = true;                                                    
>   \
> +        return;                                                              
>   \
> +    }                                                                        
>   \
> +    rte->dest.name.name ## _dest = dest;                                     
>   \
> +}
> +
> +    if ( rte->dest_mode )
> +        SET_DEST(physical, 0xf)
> +    else
> +        SET_DEST(logical, 0xff)

This reads as if the code was broken. Please add () around the outermost
{} of the macro, allowing you to put ; here. Or otherwise, just like
above for DEL_CHECK(), omit the {} as well as the final semicolon.

Furthermore - are you sure about this distinction? See e.g.
update_intremap_entry_from_ioapic() and
amd_iommu_setup_ioapic_remapping(), which unilaterally use
rte->dest.logical.logical_dest. Likewise io_apic.c's SET_DEST() users,
which generally pass "logical" for as middle argument. I thought one of
my half way recent patches (IRQ handling or AMD IOMMU work) had a remark
in it regarding such a badly documented extension, but I can't seem to
be able to find it anymore.

> @@ -482,6 +556,13 @@ void amd_iommu_ioapic_update_ire(
>          *((u32 *)&new_rte) = value;
>          /* read upper 32 bits from io-apic rte */
>          *(((u32 *)&new_rte) + 1) = __io_apic_read(apic, reg + 1);
> +
> +        if ( new_rte.delivery_mode > 1 && x2apic_enabled )

The literal 1 here wants attaching of a comment. And why the
x2apic_enabled part of the condition?

Jan

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