Re: [PATCH v4 12/14] vtd: use a bit field for root_entry

[Jan Beulich <jbeulich@suse.com>]

On 12.08.2020 15:13, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 06 August 2020 13:34
>>
>> On 04.08.2020 15:42, Paul Durrant wrote:
>>> --- a/xen/drivers/passthrough/vtd/iommu.h
>>> +++ b/xen/drivers/passthrough/vtd/iommu.h
>>> @@ -184,21 +184,28 @@
>>>  #define dma_frcd_source_id(c) (c & 0xffff)
>>>  #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */
>>>
>>> -/*
>>> - * 0: Present
>>> - * 1-11: Reserved
>>> - * 12-63: Context Ptr (12 - (haw-1))
>>> - * 64-127: Reserved
>>> - */
>>>  struct root_entry {
>>> -    u64    val;
>>> -    u64    rsvd1;
>>> +    union {
>>> +        __uint128_t val;
>>
>> I couldn't find a use of this field, and I also can't foresee any.
>> Could it be left out?
> 
> Yes, probably.
> 
>>
>>> +        struct { uint64_t lo, hi; };
>>> +        struct {
>>> +            /* 0 - 63 */
>>> +            uint64_t p:1;
>>
>> bool?
>>
> 
> I'd prefer not to. One of the points of using a bit field (at least from my PoV) is that it makes referring back to the spec. much easier, by using uint64_t types consistently and hence using bit widths that can be straightforwardly summed to give the bit offsets stated in the spec.

We've gone the suggested route for earlier struct conversions on
the AMD side, so I think we should follow suit here. See e.g.
struct amd_iommu_dte or union amd_iommu_control.

>>> --- a/xen/drivers/passthrough/vtd/x86/ats.c
>>> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
>>> @@ -74,8 +74,8 @@ int ats_device(const struct pci_dev *pdev, const struct acpi_drhd_unit *drhd)
>>>  static bool device_in_domain(const struct vtd_iommu *iommu,
>>>                               const struct pci_dev *pdev, uint16_t did)
>>>  {
>>> -    struct root_entry *root_entry;
>>> -    struct context_entry *ctxt_entry = NULL;
>>> +    struct root_entry *root_entry, *root_entries = NULL;
>>> +    struct context_entry *context_entry, *context_entries = NULL;
>>
>> Just like root_entry, root_entries doesn't look to need an initializer.
>> I'm unconvinced anyway that you now need two variables each:
>> unmap_vtd_domain_page() does quite fine with the low 12 bits not all
>> being zero, afaict.
> 
> Not passing a page aligned address into something that unmaps a page seems a little bit fragile though, e.g. if someone happened to add a check in future.

There are quite a few existing callers passing a not-page-aligned
address into unmap_domain_page(). I don't see why having one more
instance would cause any kind of issue.

Jan