[v2,2/4] PCI: move pdev_list field to common structure

Commit Message

Jan Beulich June 4, 2019, 12:42 p.m. UTC

Its management shouldn't be arch-specific, and in particular there
should be no need for special precautions when creating the special
domains.

At this occasion
- correct parenthesization of for_each_pdev(),
- stop open-coding for_each_pdev() in vPCI code.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Julien Grall June 4, 2019, 12:55 p.m. UTC | #1

Hi,

On 6/4/19 1:42 PM, Jan Beulich wrote:
> Its management shouldn't be arch-specific, and in particular there
> should be no need for special precautions when creating the special
> domains.
> 
> At this occasion
> - correct parenthesization of for_each_pdev(),
> - stop open-coding for_each_pdev() in vPCI code.

 From an Arm POV, this makes sense. With one comment below.

> @@ -476,8 +474,6 @@ struct arch_domain
>   #define has_pirq(d)        (!!((d)->arch.emulation_flags & X86_EMU_USE_PIRQ))
>   #define has_vpci(d)        (!!((d)->arch.emulation_flags & X86_EMU_VPCI))
>   
> -#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
> -
>   #define gdt_ldt_pt_idx(v) \
>         ((v)->vcpu_id >> (PAGETABLE_ORDER - GDT_LDT_VCPU_SHIFT))
>   #define pv_gdt_ptes(v) \
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -121,7 +121,9 @@ struct pci_dev {
>   };
>   
>   #define for_each_pdev(domain, pdev) \
> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
> +
> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))

This feels a bit strange to keep "arch" in the macro name when the code 
is now generic. How about "domain_has_pdevs"?

Cheers,

Jan Beulich June 4, 2019, 1:03 p.m. UTC | #2

>>> On 04.06.19 at 14:55, <[e-mail]> wrote:
> On 6/4/19 1:42 PM, Jan Beulich wrote:
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -121,7 +121,9 @@ struct pci_dev {
>>   };
>>   
>>   #define for_each_pdev(domain, pdev) \
>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>> +
>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
> 
> This feels a bit strange to keep "arch" in the macro name when the code 
> is now generic. How about "domain_has_pdevs"?

Indeed I did notice this oddity too, but if such an adjustment is
to be made then imo not in this patch. After all in turn I'd need
to change all use sites.

Jan

Julien Grall June 4, 2019, 1:05 p.m. UTC | #3

Hi Jan,

On 6/4/19 2:03 PM, Jan Beulich wrote:
>>>> On 04.06.19 at 14:55, <[e-mail]> wrote:
>> On 6/4/19 1:42 PM, Jan Beulich wrote:
>>> --- a/xen/include/xen/pci.h
>>> +++ b/xen/include/xen/pci.h
>>> @@ -121,7 +121,9 @@ struct pci_dev {
>>>    };
>>>    
>>>    #define for_each_pdev(domain, pdev) \
>>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>>> +
>>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
>>
>> This feels a bit strange to keep "arch" in the macro name when the code
>> is now generic. How about "domain_has_pdevs"?
> 
> Indeed I did notice this oddity too, but if such an adjustment is
> to be made then imo not in this patch. After all in turn I'd need
> to change all use sites.

It feels to me they are kind of tied together because has_arch_pdevs is 
an accessor of the field.

But I am happy to see this in a separate patches.

Cheers,

Jan Beulich June 4, 2019, 1:14 p.m. UTC | #4

>>> On 04.06.19 at 15:05, <[e-mail]> wrote:
> Hi Jan,
> 
> On 6/4/19 2:03 PM, Jan Beulich wrote:
>>>>> On 04.06.19 at 14:55, <[e-mail]> wrote:
>>> On 6/4/19 1:42 PM, Jan Beulich wrote:
>>>> --- a/xen/include/xen/pci.h
>>>> +++ b/xen/include/xen/pci.h
>>>> @@ -121,7 +121,9 @@ struct pci_dev {
>>>>    };
>>>>    
>>>>    #define for_each_pdev(domain, pdev) \
>>>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>>>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>>>> +
>>>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
>>>
>>> This feels a bit strange to keep "arch" in the macro name when the code
>>> is now generic. How about "domain_has_pdevs"?
>> 
>> Indeed I did notice this oddity too, but if such an adjustment is
>> to be made then imo not in this patch. After all in turn I'd need
>> to change all use sites.
> 
> It feels to me they are kind of tied together because has_arch_pdevs is 
> an accessor of the field.

In a way they are. But the name of the macro hasn't become
"wrong" by the change here. That renaming you ask for could
also have been done a year ago, if so wanted.

> But I am happy to see this in a separate patches.

FAOD - I didn't promise anything.

Jan

Julien Grall June 4, 2019, 1:42 p.m. UTC | #5

On 6/4/19 2:14 PM, Jan Beulich wrote:
>>>> On 04.06.19 at 15:05, <[e-mail]> wrote:
>> Hi Jan,
>>
>> On 6/4/19 2:03 PM, Jan Beulich wrote:
>>>>>> On 04.06.19 at 14:55, <[e-mail]> wrote:
>>>> On 6/4/19 1:42 PM, Jan Beulich wrote:
>>>>> --- a/xen/include/xen/pci.h
>>>>> +++ b/xen/include/xen/pci.h
>>>>> @@ -121,7 +121,9 @@ struct pci_dev {
>>>>>     };
>>>>>     
>>>>>     #define for_each_pdev(domain, pdev) \
>>>>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>>>>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>>>>> +
>>>>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
>>>>
>>>> This feels a bit strange to keep "arch" in the macro name when the code
>>>> is now generic. How about "domain_has_pdevs"?
>>>
>>> Indeed I did notice this oddity too, but if such an adjustment is
>>> to be made then imo not in this patch. After all in turn I'd need
>>> to change all use sites.
>>
>> It feels to me they are kind of tied together because has_arch_pdevs is
>> an accessor of the field.
> 
> In a way they are. But the name of the macro hasn't become
> "wrong" by the change here. That renaming you ask for could
> also have been done a year ago, if so wanted.

Fair enough for non-x86 code:

Acked-by: Julien Grall <[e-mail]>

Cheers,

Andrew Cooper June 4, 2019, 1:50 p.m. UTC | #6

On 04/06/2019 13:55, Julien Grall wrote:
>> @@ -476,8 +474,6 @@ struct arch_domain
>>   #define has_pirq(d)        (!!((d)->arch.emulation_flags &
>> X86_EMU_USE_PIRQ))
>>   #define has_vpci(d)        (!!((d)->arch.emulation_flags &
>> X86_EMU_VPCI))
>>   -#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
>> -
>>   #define gdt_ldt_pt_idx(v) \
>>         ((v)->vcpu_id >> (PAGETABLE_ORDER - GDT_LDT_VCPU_SHIFT))
>>   #define pv_gdt_ptes(v) \
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -121,7 +121,9 @@ struct pci_dev {
>>   };
>>     #define for_each_pdev(domain, pdev) \
>> -    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
>> +    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>> +
>> +#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
>
> This feels a bit strange to keep "arch" in the macro name when the
> code is now generic. How about "domain_has_pdevs"?

I agree that keeping arch in the name is a little weird, and this is
definitely the patch to rename it in - there are only 9 instances in the
entire codebase.

For the patch overall, Acked-by: Andrew Cooper
<[e-mail]>, however this helper ends up being named.

~Andrew
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <div class="moz-cite-prefix">On 04/06/2019 13:55, Julien Grall
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:[e-mail]">
      <blockquote type="cite">@@ -476,8 +474,6 @@ struct arch_domain
        <br>
          #define has_pirq(d)        (!!((d)-&gt;arch.emulation_flags
        &amp; X86_EMU_USE_PIRQ))
        <br>
          #define has_vpci(d)        (!!((d)-&gt;arch.emulation_flags
        &amp; X86_EMU_VPCI))
        <br>
          -#define has_arch_pdevs(d)   
        (!list_empty(&amp;(d)-&gt;arch.pdev_list))
        <br>
        -
        <br>
          #define gdt_ldt_pt_idx(v) \
        <br>
                ((v)-&gt;vcpu_id &gt;&gt; (PAGETABLE_ORDER -
        GDT_LDT_VCPU_SHIFT))
        <br>
          #define pv_gdt_ptes(v) \
        <br>
        --- a/xen/include/xen/pci.h
        <br>
        +++ b/xen/include/xen/pci.h
        <br>
        @@ -121,7 +121,9 @@ struct pci_dev {
        <br>
          };
        <br>
            #define for_each_pdev(domain, pdev) \
        <br>
        -    list_for_each_entry(pdev, &amp;(domain-&gt;arch.pdev_list),
        domain_list)
        <br>
        +    list_for_each_entry(pdev, &amp;(domain)-&gt;pdev_list,
        domain_list)
        <br>
        +
        <br>
        +#define has_arch_pdevs(d) (!list_empty(&amp;(d)-&gt;pdev_list))
        <br>
      </blockquote>
      <br>
      This feels a bit strange to keep "arch" in the macro name when the
      code is now generic. How about "domain_has_pdevs"?<br>
    </blockquote>
    <br>
    <pre>I agree that keeping arch in the name is a little weird, and this is
definitely the patch to rename it in - there are only 9 instances in the
entire codebase.

For the patch overall, Acked-by: Andrew Cooper
<a class="moz-txt-link-rfc2396E" href="mailto:[e-mail]">&lt;[e-mail]&gt;</a>, however this helper ends up being named.

~Andrew
</pre>
  </body>
</html>

Patch

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -496,7 +496,6 @@  int arch_domain_create(struct domain *d,
     uint32_t emflags;
     int rc;
 
-    INIT_LIST_HEAD(&d->arch.pdev_list);
     INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
 
     spin_lock_init(&d->arch.e820_lock);
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -291,7 +291,6 @@  void __init arch_init_memory(void)
      */
     dom_xen = domain_create(DOMID_XEN, NULL, false);
     BUG_ON(IS_ERR(dom_xen));
-    INIT_LIST_HEAD(&dom_xen->arch.pdev_list);
 
     /*
      * Initialise our DOMID_IO domain.
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -389,6 +389,10 @@  struct domain *domain_create(domid_t dom
 
     rwlock_init(&d->vnuma_rwlock);
 
+#ifdef CONFIG_HAS_PCI
+    INIT_LIST_HEAD(&d->pdev_list);
+#endif
+
     err = -ENOMEM;
     if ( !zalloc_cpumask_var(&d->dirty_cpumask) )
         goto fail;
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -315,7 +315,7 @@  static int reassign_device(struct domain
 
     if ( devfn == pdev->devfn )
     {
-        list_move(&pdev->domain_list, &target->arch.pdev_list);
+        list_move(&pdev->domain_list, &target->pdev_list);
         pdev->domain = target;
     }
 
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -467,7 +467,7 @@  static void _pci_hide_device(struct pci_
     if ( pdev->domain )
         return;
     pdev->domain = dom_xen;
-    list_add(&pdev->domain_list, &dom_xen->arch.pdev_list);
+    list_add(&pdev->domain_list, &dom_xen->pdev_list);
 }
 
 int __init pci_hide_device(unsigned int seg, unsigned int bus,
@@ -803,7 +803,7 @@  int pci_add_device(u16 seg, u8 bus, u8 d
             goto out;
         }
 
-        list_add(&pdev->domain_list, &hardware_domain->arch.pdev_list);
+        list_add(&pdev->domain_list, &hardware_domain->pdev_list);
     }
     else
         iommu_enable_device(pdev);
@@ -1153,7 +1153,7 @@  static int __hwdom_init _setup_hwdom_pci
             if ( !pdev->domain )
             {
                 pdev->domain = ctxt->d;
-                list_add(&pdev->domain_list, &ctxt->d->arch.pdev_list);
+                list_add(&pdev->domain_list, &ctxt->d->pdev_list);
                 setup_one_hwdom_device(ctxt, pdev);
             }
             else if ( pdev->domain == dom_xen )
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2453,7 +2453,7 @@  static int reassign_device_ownership(
 
     if ( devfn == pdev->devfn )
     {
-        list_move(&pdev->domain_list, &target->arch.pdev_list);
+        list_move(&pdev->domain_list, &target->pdev_list);
         pdev->domain = target;
     }
 
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -267,7 +267,7 @@  static int modify_bars(const struct pci_
      * Check for overlaps with other BARs. Note that only BARs that are
      * currently mapped (enabled) are checked for overlaps.
      */
-    list_for_each_entry(tmp, &pdev->domain->arch.pdev_list, domain_list)
+    for_each_pdev ( pdev->domain, tmp )
     {
         if ( tmp == pdev )
         {
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -282,7 +282,7 @@  void vpci_dump_msi(void)
 
         printk("vPCI MSI/MSI-X d%d\n", d->domain_id);
 
-        list_for_each_entry ( pdev, &d->arch.pdev_list, domain_list )
+        for_each_pdev ( d, pdev )
         {
             const struct vpci_msi *msi;
             const struct vpci_msix *msix;
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -298,8 +298,6 @@  struct arch_domain
 
     bool_t s3_integrity;
 
-    struct list_head pdev_list;
-
     union {
         struct pv_domain pv;
         struct hvm_domain hvm;
@@ -476,8 +474,6 @@  struct arch_domain
 #define has_pirq(d)        (!!((d)->arch.emulation_flags & X86_EMU_USE_PIRQ))
 #define has_vpci(d)        (!!((d)->arch.emulation_flags & X86_EMU_VPCI))
 
-#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
-
 #define gdt_ldt_pt_idx(v) \
       ((v)->vcpu_id >> (PAGETABLE_ORDER - GDT_LDT_VCPU_SHIFT))
 #define pv_gdt_ptes(v) \
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -121,7 +121,9 @@  struct pci_dev {
 };
 
 #define for_each_pdev(domain, pdev) \
-    list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
+    list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
+
+#define has_arch_pdevs(d) (!list_empty(&(d)->pdev_list))
 
 /*
  * The pcidevs_lock protect alldevs_list, and the assignment for the 
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -370,6 +370,10 @@  struct domain
 
     int64_t          time_offset_seconds;
 
+#ifdef CONFIG_HAS_PCI
+    struct list_head pdev_list;
+#endif
+
 #ifdef CONFIG_HAS_PASSTHROUGH
     struct domain_iommu iommu;
 #endif