Re: [Xen-devel] [PATCH 3/9] AMD/IOMMU: use bit field for IRTE

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

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