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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.08.2020 15:13, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> 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