Re: [Xen-devel] [PATCH v3 11/14] AMD/IOMMU: adjust setup of internal interrupt for x2APIC mode

[Jan Beulich <JBeulich@xxxxxxxx>]

On 22.07.2019 15:45, Andrew Cooper wrote:
> On 22/07/2019 09:43, Jan Beulich wrote:
>> On 19.07.2019 19:31, Andrew Cooper wrote:
>>> On 16/07/2019 17:39, Jan Beulich wrote:
>>>> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>>>> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>>>> @@ -416,6 +416,25 @@ union amd_iommu_ext_features {
>>>>         } flds;
>>>>     };
>>>>     
>>>> +/* x2APIC Control Registers */
>>>> +#define IOMMU_XT_INT_CTRL_MMIO_OFFSET             0x0170
>>>> +#define IOMMU_XT_PPR_INT_CTRL_MMIO_OFFSET 0x0178
>>>> +#define IOMMU_XT_GA_INT_CTRL_MMIO_OFFSET  0x0180
>>>> +
>>>> +union amd_iommu_x2apic_control {
>>>> +    uint64_t raw;
>>>> +    struct {
>>>> +        unsigned int :2;
>>>> +        unsigned int dest_mode:1;
>>>> +        unsigned int :5;
>>>> +        unsigned int dest_lo:24;
>>>> +        unsigned int vector:8;
>>>> +        unsigned int int_type:1; /* DM in IOMMU spec 3.04 */
>>>> +        unsigned int :15;
>>>> +        unsigned int dest_hi:8;
>>> Bool bitfields like you've done elsewhere in v3?
>> I'd been considering this, but decided against because of ...
>>
>> +static void set_x2apic_affinity(struct irq_desc *desc, const cpumask_t 
>> *mask)
>> +{
>> +    struct amd_iommu *iommu = desc->action->dev_id;
>> +    unsigned int dest = set_desc_affinity(desc, mask);
>> +    union amd_iommu_x2apic_control ctrl = {};
>> +    unsigned long flags;
>> +
>> +    if ( dest == BAD_APICID )
>> +        return;
>> +
>> +    msi_compose_msg(desc->arch.vector, NULL, &iommu->msi.msg);
>> +    iommu->msi.msg.dest32 = dest;
>> +
>> +    ctrl.dest_mode = MASK_EXTR(iommu->msi.msg.address_lo,
>> +                               MSI_ADDR_DESTMODE_MASK);
>> +    ctrl.int_type = MASK_EXTR(iommu->msi.msg.data,
>> +                              MSI_DATA_DELIVERY_MODE_MASK);
>>
>> ... this: We really mean a value copy here, not an "is zero" or
>> "is non-zero" one. I also think that both fields are not suitably
>> named for being boolean. In the recent re-work of struct
>> IO_APIC_route_entry (ca9310b24e) similar fields similarly were
>> left as "unsigned int". MSI's struct msg_data also falls into the
>> same category. I think if we wanted to switch to bool here, we
>> should do so everywhere at the same time (along with suitably
>> renaming fields).
> 
> Architecturally, both of these are single-bit fields, no?

Sure, but with the names we have there's no obvious indication
whether physical/logical are respectively true or false. Same
(or worse) for fixed/lowest priority, which in the LAPIC even
has further accompanying values (i.e. couldn't possibly be bool
there at all).

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