[Xen-devel] [PATCH 0/4] xen/x86: Rework inclusion between struct pirq and

[Xen-devel] [PATCH 0/4] xen/x86: Rework inclusion between struct pirq and

[Julien Grall]

Hi all,

The main goal of this series is to make easier to understand and use
struct pirq. Patch #1 and #3 are cleanups.

Cheers,

Julien Grall (4):
  xen/x86: Remove unused forward declaration in asm-x86/irq.h
  xen/char: ehci: Directly include xen/timer.h rather rely on dependency
  xen/domain: Remove #ifndef surrounding alloc_pirq_struct()
  xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

 xen/arch/arm/irq.c            |  5 +++++
 xen/arch/x86/hvm/irq.c        |  7 ++++---
 xen/arch/x86/irq.c            | 39 ++++++++++++++++++++++++-----------
 xen/common/domain.c           |  7 +------
 xen/drivers/char/ehci-dbgp.c  |  1 +
 xen/drivers/passthrough/io.c  |  1 +
 xen/include/asm-x86/hvm/irq.h | 19 +++++++++++++++++
 xen/include/asm-x86/irq.h     | 20 +++---------------
 xen/include/xen/domain.h      |  5 +++--
 9 files changed, 64 insertions(+), 40 deletions(-)

-- 
2.24.0


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

[Xen-devel] [PATCH 1/4] xen/x86: Remove unused forward declaration in asm-x86/irq.h

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

None of the prototypes within the header asm-x86/irq.h actually requires
the forward declaration of "struct pirq". So remove it.

No functional changes intended.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/asm-x86/irq.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index 7c825e9d9c..44aefc8f03 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -131,7 +131,6 @@ extern unsigned int io_apic_irqs;
 
 DECLARE_PER_CPU(unsigned int, irq_count);
 
-struct pirq;
 struct arch_pirq {
     int irq;
     union {
-- 
2.24.0


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

[Xen-devel] [PATCH 2/4] xen/char: ehci: Directly include xen/timer.h rather rely on dependency

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The ehci char driver is using timers but relying on the header
xen/timer.h to be included via asm-x86/hvm/irq.h which is not even
directly included!

Future rework will reduce the number of places where asm-x86/hvm/irq.h
will be included. Include xen/timer.h directly to avoid any breakage.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/drivers/char/ehci-dbgp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
index b6e155d17b..8124e0aad8 100644
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -10,6 +10,7 @@
 #include <xen/errno.h>
 #include <xen/pci.h>
 #include <xen/serial.h>
+#include <xen/timer.h>
 #include <asm/byteorder.h>
 #include <asm/io.h>
 #include <asm/fixmap.h>
-- 
2.24.0


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

[Xen-devel] [PATCH 3/4] xen/domain: Remove #ifndef surrounding alloc_pirq_struct()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

None of the supported architecture override alloc_pirq_struct() with
a macro. So remove the #ifdef surrounding the prototype.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/xen/domain.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 1cb205d977..89bf0a1721 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -41,9 +41,7 @@ struct vcpu *alloc_vcpu_struct(const struct domain *d);
 void free_vcpu_struct(struct vcpu *v);
 
 /* Allocate/free a PIRQ structure. */
-#ifndef alloc_pirq_struct
 struct pirq *alloc_pirq_struct(struct domain *);
-#endif
 void free_pirq_struct(void *);
 
 /*
-- 
2.24.0


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

[Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, alloc_pirq_struct() relies on the field 'arch' to be the
last member of the structure.

As this is used for computing the size of the structure, the value will
be miscomputed if a new field is added afterwards.

Such quirkiness makes quite difficult to understand how struct pirq
works. Given that struct hvm_pirq_dpci is only used in combination of a
struct pirq, we can inverse the inclusion. i.e pirq will now be
contained in struct hvm_pirq_dpci.

As the field pirq.arch.hvm.emuirq is as well HVM specific, this is now
moved in struct hvm_pirq_dpci.

There is a few side effects with this changes:
    - We now need to distinguish between PIRQ allocated for HVM and PV
      guests. This is to allow us to know what we are freeing.
    - container_of is not able to cater with const and non-const at the
      same time. So we need to introduce two macros (const and
      non-const).

Lastly all the HVM specific pirq code can now be moved in hvm/irq.h
allowing use to drop the include from irq.h. This is one less header
included treewide.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/irq.c            |  5 +++++
 xen/arch/x86/hvm/irq.c        |  7 ++++---
 xen/arch/x86/irq.c            | 39 ++++++++++++++++++++++++-----------
 xen/common/domain.c           |  7 +------
 xen/drivers/passthrough/io.c  |  1 +
 xen/include/asm-x86/hvm/irq.h | 19 +++++++++++++++++
 xen/include/asm-x86/irq.h     | 19 +++--------------
 xen/include/xen/domain.h      |  3 +++
 8 files changed, 63 insertions(+), 37 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 3877657a52..fd108ea3a5 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -582,6 +582,11 @@ struct pirq *alloc_pirq_struct(struct domain *d)
     return NULL;
 }
 
+void arch_free_pirq_struct(struct rcu_head *head)
+{
+    ASSERT_UNREACHABLE();
+}
+
 /*
  * These are all unreachable given an alloc_pirq_struct
  * which returns NULL, all callers try to lookup struct pirq first
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index c684422b24..e0bb0a8b90 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -29,7 +29,8 @@
 
 bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
 {
-    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
+    return is_hvm_domain(d) && pirq &&
+        const_pirq_dpci(pirq)->emuirq != IRQ_UNBOUND;
 }
 
 /* Must be called with hvm_domain->irq_lock hold */
@@ -396,7 +397,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
             struct pirq *info = pirq_info(d, pirq);
 
             /* if it is the first time, allocate the pirq */
-            if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
+            if ( !info || pirq_dpci(info)->emuirq == IRQ_UNBOUND )
             {
                 int rc;
 
@@ -409,7 +410,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
                 if ( !info )
                     return -EBUSY;
             }
-            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
+            else if ( pirq_dpci(info)->emuirq != IRQ_MSI_EMU )
                 return -EINVAL;
             send_guest_pirq(d, info);
             return 0;
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 310ac00a60..3e01101f88 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1286,22 +1286,37 @@ void cleanup_domain_irq_mapping(struct domain *d)
 
 struct pirq *alloc_pirq_struct(struct domain *d)
 {
-    size_t sz = is_hvm_domain(d) ? sizeof(struct pirq) :
-                                   offsetof(struct pirq, arch.hvm);
-    struct pirq *pirq = xzalloc_bytes(sz);
+    struct pirq *pirq;
 
-    if ( pirq )
+    if ( is_hvm_domain(d) )
     {
-        if ( is_hvm_domain(d) )
+        struct hvm_pirq_dpci *dpci = xzalloc(struct hvm_pirq_dpci);
+
+        if ( dpci )
         {
-            pirq->arch.hvm.emuirq = IRQ_UNBOUND;
-            pt_pirq_init(d, &pirq->arch.hvm.dpci);
+            pt_pirq_init(d, dpci);
+            pirq = dpci_pirq(dpci);
+            pirq->arch.hvm = true;
         }
+        else
+            pirq = NULL;
     }
+    else
+        pirq = xzalloc(struct pirq);
 
     return pirq;
 }
 
+void arch_free_pirq_struct(struct rcu_head *head)
+{
+    struct pirq *pirq = container_of(head, struct pirq, rcu_head);
+
+    if ( pirq->arch.hvm )
+        xfree(pirq_dpci(pirq));
+    else
+        xfree(pirq);
+}
+
 void (pirq_cleanup_check)(struct pirq *pirq, struct domain *d)
 {
     /*
@@ -1315,9 +1330,9 @@ void (pirq_cleanup_check)(struct pirq *pirq, struct domain *d)
 
     if ( is_hvm_domain(d) )
     {
-        if ( pirq->arch.hvm.emuirq != IRQ_UNBOUND )
+        if ( pirq_dpci(pirq)->emuirq != IRQ_UNBOUND )
             return;
-        if ( !pt_pirq_cleanup_check(&pirq->arch.hvm.dpci) )
+        if ( !pt_pirq_cleanup_check(pirq_dpci(pirq)) )
             return;
     }
 
@@ -2029,7 +2044,7 @@ static inline bool is_free_pirq(const struct domain *d,
                                 const struct pirq *pirq)
 {
     return !pirq || (!pirq->arch.irq && (!is_hvm_domain(d) ||
-        pirq->arch.hvm.emuirq == IRQ_UNBOUND));
+        const_pirq_dpci(pirq)->emuirq == IRQ_UNBOUND));
 }
 
 int get_free_pirq(struct domain *d, int type)
@@ -2724,7 +2739,7 @@ int map_domain_emuirq_pirq(struct domain *d, int pirq, int emuirq)
             return err;
         }
     }
-    info->arch.hvm.emuirq = emuirq;
+    pirq_dpci(info)->emuirq = emuirq;
 
     return 0;
 }
@@ -2754,7 +2769,7 @@ int unmap_domain_pirq_emuirq(struct domain *d, int pirq)
     info = pirq_info(d, pirq);
     if ( info )
     {
-        info->arch.hvm.emuirq = IRQ_UNBOUND;
+        pirq_dpci(info)->emuirq = IRQ_UNBOUND;
         pirq_cleanup_check(info, d);
     }
     if ( emuirq != IRQ_PT )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 0b1103fdb2..7f04da79e6 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1625,16 +1625,11 @@ struct pirq *pirq_get_info(struct domain *d, int pirq)
     return info;
 }
 
-static void _free_pirq_struct(struct rcu_head *head)
-{
-    xfree(container_of(head, struct pirq, rcu_head));
-}
-
 void free_pirq_struct(void *ptr)
 {
     struct pirq *pirq = ptr;
 
-    call_rcu(&pirq->rcu_head, _free_pirq_struct);
+    call_rcu(&pirq->rcu_head, arch_free_pirq_struct);
 }
 
 struct migrate_info {
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index b292e79382..e7b288b4aa 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -769,6 +769,7 @@ int pt_irq_destroy_bind(
 
 void pt_pirq_init(struct domain *d, struct hvm_pirq_dpci *dpci)
 {
+    dpci->emuirq = IRQ_UNBOUND;
     INIT_LIST_HEAD(&dpci->digl_list);
     dpci->gmsi.dest_vcpu_id = -1;
 }
diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h
index 5b7e90c179..0ccfaad53b 100644
--- a/xen/include/asm-x86/hvm/irq.h
+++ b/xen/include/asm-x86/hvm/irq.h
@@ -21,6 +21,7 @@
 #ifndef __ASM_X86_HVM_IRQ_H__
 #define __ASM_X86_HVM_IRQ_H__
 
+#include <xen/irq.h>
 #include <xen/timer.h>
 
 #include <asm/hvm/hvm.h>
@@ -171,8 +172,26 @@ struct hvm_pirq_dpci {
     struct hvm_gmsi_info gmsi;
     struct timer timer;
     struct list_head softirq_list;
+    int emuirq;
+    struct pirq pirq;
 };
 
+#define pirq_dpci(p)                                                    \
+    ((p) ? container_of(p, struct hvm_pirq_dpci, pirq) : NULL)
+#define const_pirq_dpci(p)                                              \
+    ((p) ? container_of(p, const struct hvm_pirq_dpci, pirq) : NULL)
+
+#define dpci_pirq(pd) (&(pd)->pirq)
+
+#define domain_pirq_to_emuirq(d, p) ({                                  \
+    struct pirq *__pi = pirq_info(d, p);                                \
+    __pi ? pirq_dpci(__pi)->emuirq : IRQ_UNBOUND;                       \
+})
+#define domain_emuirq_to_pirq(d, emuirq) ({                             \
+    void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
+    __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
+})
+
 void pt_pirq_init(struct domain *, struct hvm_pirq_dpci *);
 bool pt_pirq_cleanup_check(struct hvm_pirq_dpci *);
 int pt_pirq_iterate(struct domain *d,
diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index 44aefc8f03..07a63bae04 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -8,7 +8,6 @@
 #include <xen/cpumask.h>
 #include <xen/percpu.h>
 #include <xen/smp.h>
-#include <asm/hvm/irq.h>
 
 extern unsigned int nr_irqs_gsi;
 extern unsigned int nr_irqs;
@@ -133,17 +132,10 @@ DECLARE_PER_CPU(unsigned int, irq_count);
 
 struct arch_pirq {
     int irq;
-    union {
-        struct hvm_pirq {
-            int emuirq;
-            struct hvm_pirq_dpci dpci;
-        } hvm;
-    };
+    /* Is the PIRQ associated to an HVM domain? */
+    bool hvm;
 };
 
-#define pirq_dpci(pirq) ((pirq) ? &(pirq)->arch.hvm.dpci : NULL)
-#define dpci_pirq(pd) container_of(pd, struct pirq, arch.hvm.dpci)
-
 int pirq_shared(struct domain *d , int irq);
 
 int map_domain_pirq(struct domain *d, int pirq, int irq, int type,
@@ -198,12 +190,7 @@ void cleanup_domain_irq_mapping(struct domain *);
     __ret ? radix_tree_ptr_to_int(__ret) : 0;                   \
 })
 #define PIRQ_ALLOCATED -1
-#define domain_pirq_to_emuirq(d, pirq) pirq_field(d, pirq,              \
-    arch.hvm.emuirq, IRQ_UNBOUND)
-#define domain_emuirq_to_pirq(d, emuirq) ({                             \
-    void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
-    __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
-})
+
 #define IRQ_UNBOUND -1
 #define IRQ_PT -2
 #define IRQ_MSI_EMU -3
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 89bf0a1721..99aea630d4 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -42,6 +42,9 @@ void free_vcpu_struct(struct vcpu *v);
 
 /* Allocate/free a PIRQ structure. */
 struct pirq *alloc_pirq_struct(struct domain *);
+
+/* Per-arch callback used by the RCU */
+void arch_free_pirq_struct(struct rcu_head *head);
 void free_pirq_struct(void *);
 
 /*
-- 
2.24.0


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

Re: [Xen-devel] [PATCH 1/4] xen/x86: Remove unused forward declaration in asm-x86/irq.h

[Jan Beulich]

On 13.01.2020 22:33, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> None of the prototypes within the header asm-x86/irq.h actually requires
> the forward declaration of "struct pirq". So remove it.
> 
> No functional changes intended.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

It is generally nice to identify if this was missed cleanup (the
need indeed went away in 4.12), or if such has never really been
needed.

Jan

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

Re: [Xen-devel] [PATCH 2/4] xen/char: ehci: Directly include xen/timer.h rather rely on dependency

[Jan Beulich]

On 13.01.2020 22:33, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The ehci char driver is using timers but relying on the header
> xen/timer.h to be included via asm-x86/hvm/irq.h which is not even
> directly included!
> 
> Future rework will reduce the number of places where asm-x86/hvm/irq.h
> will be included. Include xen/timer.h directly to avoid any breakage.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

Re: [Xen-devel] [PATCH 3/4] xen/domain: Remove #ifndef surrounding alloc_pirq_struct()

[Jan Beulich]

On 13.01.2020 22:33, Julien Grall wrote:
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -41,9 +41,7 @@ struct vcpu *alloc_vcpu_struct(const struct domain *d);
>  void free_vcpu_struct(struct vcpu *v);
>  
>  /* Allocate/free a PIRQ structure. */
> -#ifndef alloc_pirq_struct
>  struct pirq *alloc_pirq_struct(struct domain *);
> -#endif
>  void free_pirq_struct(void *);

Is this really a helpful change. Back then ia64 had a #define for
this, and a future port may want to do so as well. Is there
anything actively problematic with leaving this untouched?

Jan

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

Re: [Xen-devel] [PATCH 1/4] xen/x86: Remove unused forward declaration in asm-x86/irq.h

[Julien Grall]

Hi Jan,

On 14/01/2020 09:31, Jan Beulich wrote:
> On 13.01.2020 22:33, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> None of the prototypes within the header asm-x86/irq.h actually requires
>> the forward declaration of "struct pirq". So remove it.
>>
>> No functional changes intended.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> It is generally nice to identify if this was missed cleanup (the
> need indeed went away in 4.12), or if such has never really been
> needed.

Yes it is nice to have but this is a best effort basis for cleanup. They 
are not fixes and therefore not going to be backported. So I don't feel 
the need to browse more than 15 years worth of history and check whether 
a cleanup were missed.

What matter for cleanup is the current context and whether they make 
sense now.

Anyway, I would be happy to add a word in the commit message if you 
point me to the commit removing the need.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 1/4] xen/x86: Remove unused forward declaration in asm-x86/irq.h

[Jan Beulich]

On 14.01.2020 11:05, Julien Grall wrote:
> On 14/01/2020 09:31, Jan Beulich wrote:
>> On 13.01.2020 22:33, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> None of the prototypes within the header asm-x86/irq.h actually requires
>>> the forward declaration of "struct pirq". So remove it.
>>>
>>> No functional changes intended.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> It is generally nice to identify if this was missed cleanup (the
>> need indeed went away in 4.12), or if such has never really been
>> needed.
> 
> Yes it is nice to have but this is a best effort basis for cleanup. They 
> are not fixes and therefore not going to be backported. So I don't feel 
> the need to browse more than 15 years worth of history and check whether 
> a cleanup were missed.

15 years? It took me less than a minute (a single grep) to figure
the version this became unnecessary in. And I wouldn't ask for
such on a pretty simple patch like this one if I anticipated a
lot of effort to be needed.

> What matter for cleanup is the current context and whether they make 
> sense now.

I disagree. History often helps understand whether something was done
in a certain way without an obvious (from current state of things)
reason.

> Anyway, I would be happy to add a word in the commit message if you 
> point me to the commit removing the need.

Me having told you the version it disappeared in would have made this
very low effort to you. Anyway: c759fb5bc303 ("x86: move
hvm_domain_use_pirq to hvm files").

Jan

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

Re: [Xen-devel] [PATCH 1/4] xen/x86: Remove unused forward declaration in asm-x86/irq.h

[Julien Grall]

On 14/01/2020 10:15, Jan Beulich wrote:
> On 14.01.2020 11:05, Julien Grall wrote:
>> On 14/01/2020 09:31, Jan Beulich wrote:
>>> On 13.01.2020 22:33, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> None of the prototypes within the header asm-x86/irq.h actually requires
>>>> the forward declaration of "struct pirq". So remove it.
>>>>
>>>> No functional changes intended.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> It is generally nice to identify if this was missed cleanup (the
>>> need indeed went away in 4.12), or if such has never really been
>>> needed.
>>
>> Yes it is nice to have but this is a best effort basis for cleanup. They
>> are not fixes and therefore not going to be backported. So I don't feel
>> the need to browse more than 15 years worth of history and check whether
>> a cleanup were missed.
> 
> 15 years? It took me less than a minute (a single grep) to figure
> the version this became unnecessary in. And I wouldn't ask for
> such on a pretty simple patch like this one if I anticipated a
> lot of effort to be needed.

My comment is generic to cleanup... As I said, this is a best effort 
basis. Maybe I could have done it here, but I didn't feel the need to do it.

> 
>> What matter for cleanup is the current context and whether they make
>> sense now.
> 
> I disagree. History often helps understand whether something was done
> in a certain way without an obvious (from current state of things)
> reason.
> 
>> Anyway, I would be happy to add a word in the commit message if you
>> point me to the commit removing the need.
> 
> Me having told you the version it disappeared in would have made this
> very low effort to you. 
That's pretty much withdrawing knowledge you may have. I don't think 
this is suitable mindset for a collaborative project. The more you have 
more knowledge than me on the x86/pirq part.

Anyway, thank you for thinking I put little effort on this patch... 
That's a pretty great way to encourage people...

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 3/4] xen/domain: Remove #ifndef surrounding alloc_pirq_struct()

[Julien Grall]

Hi Jan,

On 14/01/2020 09:37, Jan Beulich wrote:
> On 13.01.2020 22:33, Julien Grall wrote:
>> --- a/xen/include/xen/domain.h
>> +++ b/xen/include/xen/domain.h
>> @@ -41,9 +41,7 @@ struct vcpu *alloc_vcpu_struct(const struct domain *d);
>>   void free_vcpu_struct(struct vcpu *v);
>>   
>>   /* Allocate/free a PIRQ structure. */
>> -#ifndef alloc_pirq_struct
>>   struct pirq *alloc_pirq_struct(struct domain *);
>> -#endif
>>   void free_pirq_struct(void *);
> 
> Is this really a helpful change. Back then ia64 had a #define for
> this, and a future port may want to do so as well.

I did notice it was used by ia64 but I am unconvinced this is going to 
be used in the future. Most likely because there is singler caller for 
alloc_pirq_struct() and this is not a hot path. So using a static 
inline/macro is not really a good solution here.

> Is there anything actively problematic with leaving this untouched?
Yes, this doesn't reduce the amount of unused code we have and will 
unlikely to be used in the future. If we really need it then we can 
re-introduce it.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

[Jan Beulich]

On 13.01.2020 22:33, Julien Grall wrote:
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -29,7 +29,8 @@
>  
>  bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
>  {
> -    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
> +    return is_hvm_domain(d) && pirq &&
> +        const_pirq_dpci(pirq)->emuirq != IRQ_UNBOUND;
>  }
>  
>  /* Must be called with hvm_domain->irq_lock hold */
> @@ -396,7 +397,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>              struct pirq *info = pirq_info(d, pirq);
>  
>              /* if it is the first time, allocate the pirq */
> -            if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
> +            if ( !info || pirq_dpci(info)->emuirq == IRQ_UNBOUND )
>              {
>                  int rc;
>  
> @@ -409,7 +410,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>                  if ( !info )
>                      return -EBUSY;
>              }
> -            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
> +            else if ( pirq_dpci(info)->emuirq != IRQ_MSI_EMU )
>                  return -EINVAL;
>              send_guest_pirq(d, info);
>              return 0;

All of these uses (and others further down) make pretty clear
that the emuirq field doesn't belong in the structure you put it
in - the 'd' in dpci stands for "direct" afaik, and the field is
for a certain variant of emulation of interrupt delivery into
guests, i.e. not really pass-through focused at all.

> @@ -171,8 +172,26 @@ struct hvm_pirq_dpci {
>      struct hvm_gmsi_info gmsi;
>      struct timer timer;
>      struct list_head softirq_list;
> +    int emuirq;
> +    struct pirq pirq;
>  };
>  
> +#define pirq_dpci(p)                                                    \
> +    ((p) ? container_of(p, struct hvm_pirq_dpci, pirq) : NULL)
> +#define const_pirq_dpci(p)                                              \
> +    ((p) ? container_of(p, const struct hvm_pirq_dpci, pirq) : NULL)
> +
> +#define dpci_pirq(pd) (&(pd)->pirq)
> +
> +#define domain_pirq_to_emuirq(d, p) ({                                  \
> +    struct pirq *__pi = pirq_info(d, p);                                \
> +    __pi ? pirq_dpci(__pi)->emuirq : IRQ_UNBOUND;                       \
> +})
> +#define domain_emuirq_to_pirq(d, emuirq) ({                             \
> +    void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
> +    __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
> +})

While for the latter you merely move the bogus double-leading-
underscore macro local variable (which on this occasion I'd
like to ask anyway to be changed), you actively introduce a
new similar name space violation in the domain_pirq_to_emuirq().

> @@ -133,17 +132,10 @@ DECLARE_PER_CPU(unsigned int, irq_count);
>  
>  struct arch_pirq {
>      int irq;
> -    union {
> -        struct hvm_pirq {
> -            int emuirq;
> -            struct hvm_pirq_dpci dpci;
> -        } hvm;
> -    };
> +    /* Is the PIRQ associated to an HVM domain? */
> +    bool hvm;

It looks like this field is needed for only arch_free_pirq_struct().
As it'll make a difference to struct pirq's size, can you not get
away without it? All (perhaps indirect) callers of the function
know the domain, after all.

Jan

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

Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

[Julien Grall]

Hi Jan,

On 14/01/2020 16:08, Jan Beulich wrote:
> On 13.01.2020 22:33, Julien Grall wrote:
>> --- a/xen/arch/x86/hvm/irq.c
>> +++ b/xen/arch/x86/hvm/irq.c
>> @@ -29,7 +29,8 @@
>>   
>>   bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
>>   {
>> -    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
>> +    return is_hvm_domain(d) && pirq &&
>> +        const_pirq_dpci(pirq)->emuirq != IRQ_UNBOUND;
>>   }
>>   
>>   /* Must be called with hvm_domain->irq_lock hold */
>> @@ -396,7 +397,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>               struct pirq *info = pirq_info(d, pirq);
>>   
>>               /* if it is the first time, allocate the pirq */
>> -            if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
>> +            if ( !info || pirq_dpci(info)->emuirq == IRQ_UNBOUND )
>>               {
>>                   int rc;
>>   
>> @@ -409,7 +410,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>                   if ( !info )
>>                       return -EBUSY;
>>               }
>> -            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
>> +            else if ( pirq_dpci(info)->emuirq != IRQ_MSI_EMU )
>>                   return -EINVAL;
>>               send_guest_pirq(d, info);
>>               return 0;
> 
> All of these uses (and others further down) make pretty clear
> that the emuirq field doesn't belong in the structure you put it
> in - the 'd' in dpci stands for "direct" afaik, and the field is
> for a certain variant of emulation of interrupt delivery into
> guests, i.e. not really pass-through focused at all.

I am happy to keep emuirq in struct pirq if you are happy with slightly 
increasing the size allocated on PV.

The main thing I want to get rid of is the weird allocation size we do 
today.

> 
>> @@ -171,8 +172,26 @@ struct hvm_pirq_dpci {
>>       struct hvm_gmsi_info gmsi;
>>       struct timer timer;
>>       struct list_head softirq_list;
>> +    int emuirq;
>> +    struct pirq pirq;
>>   };
>>   
>> +#define pirq_dpci(p)                                                    \
>> +    ((p) ? container_of(p, struct hvm_pirq_dpci, pirq) : NULL)
>> +#define const_pirq_dpci(p)                                              \
>> +    ((p) ? container_of(p, const struct hvm_pirq_dpci, pirq) : NULL)
>> +
>> +#define dpci_pirq(pd) (&(pd)->pirq)
>> +
>> +#define domain_pirq_to_emuirq(d, p) ({                                  \
>> +    struct pirq *__pi = pirq_info(d, p);                                \
>> +    __pi ? pirq_dpci(__pi)->emuirq : IRQ_UNBOUND;                       \
>> +})
>> +#define domain_emuirq_to_pirq(d, emuirq) ({                             \
>> +    void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
>> +    __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
>> +})
> 
> While for the latter you merely move the bogus double-leading-
> underscore macro local variable (which on this occasion I'd
> like to ask anyway to be changed), you actively introduce a
> new similar name space violation in the domain_pirq_to_emuirq().

AFAIK, there is nothing in the coding style forbidding your "bogus" 
naming. So I just followed the rest of the code.

> 
>> @@ -133,17 +132,10 @@ DECLARE_PER_CPU(unsigned int, irq_count);
>>   
>>   struct arch_pirq {
>>       int irq;
>> -    union {
>> -        struct hvm_pirq {
>> -            int emuirq;
>> -            struct hvm_pirq_dpci dpci;
>> -        } hvm;
>> -    };
>> +    /* Is the PIRQ associated to an HVM domain? */
>> +    bool hvm;
> 
> It looks like this field is needed for only arch_free_pirq_struct().
> As it'll make a difference to struct pirq's size, can you not get
> away without it? All (perhaps indirect) callers of the function
> know the domain, after all.

The free is done through an RCU callback with no extra parameters to 
tell how it can be freed.

The only way I can think of to get rid of the field is to introduce two 
different callback for the free. We would use a different callback 
depending on the domain type.

How does that sound?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

[Jan Beulich]

On 14.01.2020 17:26, Julien Grall wrote:
> On 14/01/2020 16:08, Jan Beulich wrote:
>> On 13.01.2020 22:33, Julien Grall wrote:
>>> --- a/xen/arch/x86/hvm/irq.c
>>> +++ b/xen/arch/x86/hvm/irq.c
>>> @@ -29,7 +29,8 @@
>>>   
>>>   bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
>>>   {
>>> -    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
>>> +    return is_hvm_domain(d) && pirq &&
>>> +        const_pirq_dpci(pirq)->emuirq != IRQ_UNBOUND;
>>>   }
>>>   
>>>   /* Must be called with hvm_domain->irq_lock hold */
>>> @@ -396,7 +397,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>>               struct pirq *info = pirq_info(d, pirq);
>>>   
>>>               /* if it is the first time, allocate the pirq */
>>> -            if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
>>> +            if ( !info || pirq_dpci(info)->emuirq == IRQ_UNBOUND )
>>>               {
>>>                   int rc;
>>>   
>>> @@ -409,7 +410,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>>                   if ( !info )
>>>                       return -EBUSY;
>>>               }
>>> -            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
>>> +            else if ( pirq_dpci(info)->emuirq != IRQ_MSI_EMU )
>>>                   return -EINVAL;
>>>               send_guest_pirq(d, info);
>>>               return 0;
>>
>> All of these uses (and others further down) make pretty clear
>> that the emuirq field doesn't belong in the structure you put it
>> in - the 'd' in dpci stands for "direct" afaik, and the field is
>> for a certain variant of emulation of interrupt delivery into
>> guests, i.e. not really pass-through focused at all.
> 
> I am happy to keep emuirq in struct pirq if you are happy with slightly 
> increasing the size allocated on PV.
> 
> The main thing I want to get rid of is the weird allocation size we do 
> today.

While I understand this, to be honest I'd rather not see the size
grow for no good (to PV) reason. I don't think the current model is
_this_ bad. But if you really want to push for it, why can't the
two parts continue to live in a wrapper HVM structure, just like
they do today?

>>> @@ -171,8 +172,26 @@ struct hvm_pirq_dpci {
>>>       struct hvm_gmsi_info gmsi;
>>>       struct timer timer;
>>>       struct list_head softirq_list;
>>> +    int emuirq;
>>> +    struct pirq pirq;
>>>   };
>>>   
>>> +#define pirq_dpci(p)                                                    \
>>> +    ((p) ? container_of(p, struct hvm_pirq_dpci, pirq) : NULL)
>>> +#define const_pirq_dpci(p)                                              \
>>> +    ((p) ? container_of(p, const struct hvm_pirq_dpci, pirq) : NULL)
>>> +
>>> +#define dpci_pirq(pd) (&(pd)->pirq)
>>> +
>>> +#define domain_pirq_to_emuirq(d, p) ({                                  \
>>> +    struct pirq *__pi = pirq_info(d, p);                                \
>>> +    __pi ? pirq_dpci(__pi)->emuirq : IRQ_UNBOUND;                       \
>>> +})
>>> +#define domain_emuirq_to_pirq(d, emuirq) ({                             \
>>> +    void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
>>> +    __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
>>> +})
>>
>> While for the latter you merely move the bogus double-leading-
>> underscore macro local variable (which on this occasion I'd
>> like to ask anyway to be changed), you actively introduce a
>> new similar name space violation in the domain_pirq_to_emuirq().
> 
> AFAIK, there is nothing in the coding style forbidding your "bogus" 
> naming. So I just followed the rest of the code.

Our coding style document is not to re-iterate C standard rules,
I think, and hence yes, you won't find anything to this effect
there.

>>> @@ -133,17 +132,10 @@ DECLARE_PER_CPU(unsigned int, irq_count);
>>>   
>>>   struct arch_pirq {
>>>       int irq;
>>> -    union {
>>> -        struct hvm_pirq {
>>> -            int emuirq;
>>> -            struct hvm_pirq_dpci dpci;
>>> -        } hvm;
>>> -    };
>>> +    /* Is the PIRQ associated to an HVM domain? */
>>> +    bool hvm;
>>
>> It looks like this field is needed for only arch_free_pirq_struct().
>> As it'll make a difference to struct pirq's size, can you not get
>> away without it? All (perhaps indirect) callers of the function
>> know the domain, after all.
> 
> The free is done through an RCU callback with no extra parameters to 
> tell how it can be freed.
> 
> The only way I can think of to get rid of the field is to introduce two 
> different callback for the free. We would use a different callback 
> depending on the domain type.
> 
> How does that sound?

That's exactly what I was thinking of as a possible solution.

Jan

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

Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

[Julien Grall]

Hi,

On 14/01/2020 16:50, Jan Beulich wrote:
> On 14.01.2020 17:26, Julien Grall wrote:
>> On 14/01/2020 16:08, Jan Beulich wrote:
>>> On 13.01.2020 22:33, Julien Grall wrote:
>>>> --- a/xen/arch/x86/hvm/irq.c
>>>> +++ b/xen/arch/x86/hvm/irq.c
>>>> @@ -29,7 +29,8 @@
>>>>    
>>>>    bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
>>>>    {
>>>> -    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
>>>> +    return is_hvm_domain(d) && pirq &&
>>>> +        const_pirq_dpci(pirq)->emuirq != IRQ_UNBOUND;
>>>>    }
>>>>    
>>>>    /* Must be called with hvm_domain->irq_lock hold */
>>>> @@ -396,7 +397,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>>>                struct pirq *info = pirq_info(d, pirq);
>>>>    
>>>>                /* if it is the first time, allocate the pirq */
>>>> -            if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
>>>> +            if ( !info || pirq_dpci(info)->emuirq == IRQ_UNBOUND )
>>>>                {
>>>>                    int rc;
>>>>    
>>>> @@ -409,7 +410,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>>>                    if ( !info )
>>>>                        return -EBUSY;
>>>>                }
>>>> -            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
>>>> +            else if ( pirq_dpci(info)->emuirq != IRQ_MSI_EMU )
>>>>                    return -EINVAL;
>>>>                send_guest_pirq(d, info);
>>>>                return 0;
>>>
>>> All of these uses (and others further down) make pretty clear
>>> that the emuirq field doesn't belong in the structure you put it
>>> in - the 'd' in dpci stands for "direct" afaik, and the field is
>>> for a certain variant of emulation of interrupt delivery into
>>> guests, i.e. not really pass-through focused at all.
>>
>> I am happy to keep emuirq in struct pirq if you are happy with slightly
>> increasing the size allocated on PV.
>>
>> The main thing I want to get rid of is the weird allocation size we do
>> today.
> 
> While I understand this, to be honest I'd rather not see the size
> grow for no good (to PV) reason. I don't think the current model is
> _this_ bad.

Well, I did lost two days debugging a problem because of the allocation 
(the memory were getting corrupted randomly). The comment you added may 
help to avoid this problem but I still think that trying to allocate 
half a pirq is a pretty bad idea.

> But if you really want to push for it, why can't the
> two parts continue to live in a wrapper HVM structure, just like
> they do today?

I am not sure what you are suggesting here. Could you extend your thought?

> 
>>>> @@ -171,8 +172,26 @@ struct hvm_pirq_dpci {
>>>>        struct hvm_gmsi_info gmsi;
>>>>        struct timer timer;
>>>>        struct list_head softirq_list;
>>>> +    int emuirq;
>>>> +    struct pirq pirq;
>>>>    };
>>>>    
>>>> +#define pirq_dpci(p)                                                    \
>>>> +    ((p) ? container_of(p, struct hvm_pirq_dpci, pirq) : NULL)
>>>> +#define const_pirq_dpci(p)                                              \
>>>> +    ((p) ? container_of(p, const struct hvm_pirq_dpci, pirq) : NULL)
>>>> +
>>>> +#define dpci_pirq(pd) (&(pd)->pirq)
>>>> +
>>>> +#define domain_pirq_to_emuirq(d, p) ({                                  \
>>>> +    struct pirq *__pi = pirq_info(d, p);                                \
>>>> +    __pi ? pirq_dpci(__pi)->emuirq : IRQ_UNBOUND;                       \
>>>> +})
>>>> +#define domain_emuirq_to_pirq(d, emuirq) ({                             \
>>>> +    void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
>>>> +    __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
>>>> +})
>>>
>>> While for the latter you merely move the bogus double-leading-
>>> underscore macro local variable (which on this occasion I'd
>>> like to ask anyway to be changed), you actively introduce a
>>> new similar name space violation in the domain_pirq_to_emuirq().
>>
>> AFAIK, there is nothing in the coding style forbidding your "bogus"
>> naming. So I just followed the rest of the code.
> 
> Our coding style document is not to re-iterate C standard rules,
> I think, and hence yes, you won't find anything to this effect
> there.

The fact such code has been added in Xen in the past clearly shows that 
the coding style is not sufficient to back your point here.

So rather than complaining that I don't follow an unwritten rule, you 
could have suggested it. This would have came accross as less rude.

Anyway, I will update it.

> 
>>>> @@ -133,17 +132,10 @@ DECLARE_PER_CPU(unsigned int, irq_count);
>>>>    
>>>>    struct arch_pirq {
>>>>        int irq;
>>>> -    union {
>>>> -        struct hvm_pirq {
>>>> -            int emuirq;
>>>> -            struct hvm_pirq_dpci dpci;
>>>> -        } hvm;
>>>> -    };
>>>> +    /* Is the PIRQ associated to an HVM domain? */
>>>> +    bool hvm;
>>>
>>> It looks like this field is needed for only arch_free_pirq_struct().
>>> As it'll make a difference to struct pirq's size, can you not get
>>> away without it? All (perhaps indirect) callers of the function
>>> know the domain, after all.
>>
>> The free is done through an RCU callback with no extra parameters to
>> tell how it can be freed.
>>
>> The only way I can think of to get rid of the field is to introduce two
>> different callback for the free. We would use a different callback
>> depending on the domain type.
>>
>> How does that sound?
> 
> That's exactly what I was thinking of as a possible solution.

I will have a look.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

[Jan Beulich]

On 14.01.2020 18:03, Julien Grall wrote:
> On 14/01/2020 16:50, Jan Beulich wrote:
>> On 14.01.2020 17:26, Julien Grall wrote:
>>> On 14/01/2020 16:08, Jan Beulich wrote:
>>>> On 13.01.2020 22:33, Julien Grall wrote:
>>>>> --- a/xen/arch/x86/hvm/irq.c
>>>>> +++ b/xen/arch/x86/hvm/irq.c
>>>>> @@ -29,7 +29,8 @@
>>>>>    
>>>>>    bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
>>>>>    {
>>>>> -    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
>>>>> +    return is_hvm_domain(d) && pirq &&
>>>>> +        const_pirq_dpci(pirq)->emuirq != IRQ_UNBOUND;
>>>>>    }
>>>>>    
>>>>>    /* Must be called with hvm_domain->irq_lock hold */
>>>>> @@ -396,7 +397,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>>>>                struct pirq *info = pirq_info(d, pirq);
>>>>>    
>>>>>                /* if it is the first time, allocate the pirq */
>>>>> -            if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
>>>>> +            if ( !info || pirq_dpci(info)->emuirq == IRQ_UNBOUND )
>>>>>                {
>>>>>                    int rc;
>>>>>    
>>>>> @@ -409,7 +410,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>>>>                    if ( !info )
>>>>>                        return -EBUSY;
>>>>>                }
>>>>> -            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
>>>>> +            else if ( pirq_dpci(info)->emuirq != IRQ_MSI_EMU )
>>>>>                    return -EINVAL;
>>>>>                send_guest_pirq(d, info);
>>>>>                return 0;
>>>>
>>>> All of these uses (and others further down) make pretty clear
>>>> that the emuirq field doesn't belong in the structure you put it
>>>> in - the 'd' in dpci stands for "direct" afaik, and the field is
>>>> for a certain variant of emulation of interrupt delivery into
>>>> guests, i.e. not really pass-through focused at all.
>>>
>>> I am happy to keep emuirq in struct pirq if you are happy with slightly
>>> increasing the size allocated on PV.
>>>
>>> The main thing I want to get rid of is the weird allocation size we do
>>> today.
>>
>> While I understand this, to be honest I'd rather not see the size
>> grow for no good (to PV) reason. I don't think the current model is
>> _this_ bad.
> 
> Well, I did lost two days debugging a problem because of the allocation 
> (the memory were getting corrupted randomly). The comment you added may 
> help to avoid this problem but I still think that trying to allocate 
> half a pirq is a pretty bad idea.

To me, not significantly different from your container_of() approach.

>> But if you really want to push for it, why can't the
>> two parts continue to live in a wrapper HVM structure, just like
>> they do today?
> 
> I am not sure what you are suggesting here. Could you extend your thought?

Right now we have

struct arch_pirq {
    int irq;
    union {
        struct hvm_pirq {
            int emuirq;
            struct hvm_pirq_dpci dpci;
        } hvm;
    };
};

What I'm suggesting is to keep

struct hvm_pirq {
     int emuirq;
     struct hvm_pirq_dpci dpci;
};

and add struct arch_pirq into there. Arguably it could even
be first in there, thus allowing xfree() to free the whole
thing no matter whether passed a struct hvm_pirq * or a
struct arch_pirq * (and eliminating the need for a per-
arch abstraction of the freeing).

>>>>> @@ -171,8 +172,26 @@ struct hvm_pirq_dpci {
>>>>>        struct hvm_gmsi_info gmsi;
>>>>>        struct timer timer;
>>>>>        struct list_head softirq_list;
>>>>> +    int emuirq;
>>>>> +    struct pirq pirq;
>>>>>    };
>>>>>    
>>>>> +#define pirq_dpci(p)                                                    \
>>>>> +    ((p) ? container_of(p, struct hvm_pirq_dpci, pirq) : NULL)
>>>>> +#define const_pirq_dpci(p)                                              \
>>>>> +    ((p) ? container_of(p, const struct hvm_pirq_dpci, pirq) : NULL)
>>>>> +
>>>>> +#define dpci_pirq(pd) (&(pd)->pirq)
>>>>> +
>>>>> +#define domain_pirq_to_emuirq(d, p) ({                                  \
>>>>> +    struct pirq *__pi = pirq_info(d, p);                                \
>>>>> +    __pi ? pirq_dpci(__pi)->emuirq : IRQ_UNBOUND;                       \
>>>>> +})
>>>>> +#define domain_emuirq_to_pirq(d, emuirq) ({                             \
>>>>> +    void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
>>>>> +    __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
>>>>> +})
>>>>
>>>> While for the latter you merely move the bogus double-leading-
>>>> underscore macro local variable (which on this occasion I'd
>>>> like to ask anyway to be changed), you actively introduce a
>>>> new similar name space violation in the domain_pirq_to_emuirq().
>>>
>>> AFAIK, there is nothing in the coding style forbidding your "bogus"
>>> naming. So I just followed the rest of the code.
>>
>> Our coding style document is not to re-iterate C standard rules,
>> I think, and hence yes, you won't find anything to this effect
>> there.
> 
> The fact such code has been added in Xen in the past clearly shows that 
> the coding style is not sufficient to back your point here.
> 
> So rather than complaining that I don't follow an unwritten rule, you 
> could have suggested it. This would have came accross as less rude.

If anything I said came across as rude, I'd like to apologize.
As an explanation (not an excuse), please be aware that I've
had to request changes to comply to name space rules far too
often that I would recall towards whom I did send these, or
that I would assume any of the regular contributors could in
fact never have noticed this so far.

I do insist on my point though that we, earning our money with
programming, and hence probably calling ourselves "professional
programmers", should know and honor basic principles of the
standards of languages we're using in our day to day work. The
fact that code violating this had been added to Xen in the past
does not make this any better; the excuse there may well be
that it started out as a research project, where such
considerations may not have mattered all this much. (FAOD I
explicitly said "basic principles" - I don't expect everyone to
know every corner case.)

Do you want me to submit a patch adding something like "It
probably goes without saying that the underlying language
standards or specifications are to be honored", perhaps close
to the top of ./CODING_STYLE?

Jan

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

Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

[Julien Grall]

Hi Jan,

On 15/01/2020 10:44, Jan Beulich wrote:
> On 14.01.2020 18:03, Julien Grall wrote:
>> On 14/01/2020 16:50, Jan Beulich wrote:
>>> On 14.01.2020 17:26, Julien Grall wrote:
>>>> On 14/01/2020 16:08, Jan Beulich wrote:
>>>>> On 13.01.2020 22:33, Julien Grall wrote:
>>>>>> --- a/xen/arch/x86/hvm/irq.c
>>>>>> +++ b/xen/arch/x86/hvm/irq.c
>>>>>> @@ -29,7 +29,8 @@
>>>>>>     
>>>>>>     bool hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
>>>>>>     {
>>>>>> -    return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != IRQ_UNBOUND;
>>>>>> +    return is_hvm_domain(d) && pirq &&
>>>>>> +        const_pirq_dpci(pirq)->emuirq != IRQ_UNBOUND;
>>>>>>     }
>>>>>>     
>>>>>>     /* Must be called with hvm_domain->irq_lock hold */
>>>>>> @@ -396,7 +397,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>>>>>                 struct pirq *info = pirq_info(d, pirq);
>>>>>>     
>>>>>>                 /* if it is the first time, allocate the pirq */
>>>>>> -            if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
>>>>>> +            if ( !info || pirq_dpci(info)->emuirq == IRQ_UNBOUND )
>>>>>>                 {
>>>>>>                     int rc;
>>>>>>     
>>>>>> @@ -409,7 +410,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
>>>>>>                     if ( !info )
>>>>>>                         return -EBUSY;
>>>>>>                 }
>>>>>> -            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
>>>>>> +            else if ( pirq_dpci(info)->emuirq != IRQ_MSI_EMU )
>>>>>>                     return -EINVAL;
>>>>>>                 send_guest_pirq(d, info);
>>>>>>                 return 0;
>>>>>
>>>>> All of these uses (and others further down) make pretty clear
>>>>> that the emuirq field doesn't belong in the structure you put it
>>>>> in - the 'd' in dpci stands for "direct" afaik, and the field is
>>>>> for a certain variant of emulation of interrupt delivery into
>>>>> guests, i.e. not really pass-through focused at all.
>>>>
>>>> I am happy to keep emuirq in struct pirq if you are happy with slightly
>>>> increasing the size allocated on PV.
>>>>
>>>> The main thing I want to get rid of is the weird allocation size we do
>>>> today.
>>>
>>> While I understand this, to be honest I'd rather not see the size
>>> grow for no good (to PV) reason. I don't think the current model is
>>> _this_ bad.
>>
>> Well, I did lost two days debugging a problem because of the allocation
>> (the memory were getting corrupted randomly). The comment you added may
>> help to avoid this problem but I still think that trying to allocate
>> half a pirq is a pretty bad idea.
> 
> To me, not significantly different from your container_of() approach.

I guess it is a matter of perspective. The implementation of alloc/free 
is not much better, but a user trying to add a new field will not fall 
into the trap again (comments can often be overlooked).

> 
>>> But if you really want to push for it, why can't the
>>> two parts continue to live in a wrapper HVM structure, just like
>>> they do today?
>>
>> I am not sure what you are suggesting here. Could you extend your thought?
> 
> Right now we have
> 
> struct arch_pirq {
>      int irq;
>      union {
>          struct hvm_pirq {
>              int emuirq;
>              struct hvm_pirq_dpci dpci;
>          } hvm;
>      };
> };
> 
> What I'm suggesting is to keep
> 
> struct hvm_pirq {
>       int emuirq;
>       struct hvm_pirq_dpci dpci;
> };
> 
> and add struct arch_pirq into there. Arguably it could even
> be first in there, thus allowing xfree() to free the whole
> thing no matter whether passed a struct hvm_pirq * or a
> struct arch_pirq * (and eliminating the need for a per-
> arch abstraction of the freeing).

I guess you mean struct pirq instead of struct arch_pirq. If so, I will 
have a look. The code should be much cleaner than what I have submitted.

> 
>>>>>> @@ -171,8 +172,26 @@ struct hvm_pirq_dpci {
>>>>>>         struct hvm_gmsi_info gmsi;
>>>>>>         struct timer timer;
>>>>>>         struct list_head softirq_list;
>>>>>> +    int emuirq;
>>>>>> +    struct pirq pirq;
>>>>>>     };
>>>>>>     
>>>>>> +#define pirq_dpci(p)                                                    \
>>>>>> +    ((p) ? container_of(p, struct hvm_pirq_dpci, pirq) : NULL)
>>>>>> +#define const_pirq_dpci(p)                                              \
>>>>>> +    ((p) ? container_of(p, const struct hvm_pirq_dpci, pirq) : NULL)
>>>>>> +
>>>>>> +#define dpci_pirq(pd) (&(pd)->pirq)
>>>>>> +
>>>>>> +#define domain_pirq_to_emuirq(d, p) ({                                  \
>>>>>> +    struct pirq *__pi = pirq_info(d, p);                                \
>>>>>> +    __pi ? pirq_dpci(__pi)->emuirq : IRQ_UNBOUND;                       \
>>>>>> +})
>>>>>> +#define domain_emuirq_to_pirq(d, emuirq) ({                             \
>>>>>> +    void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
>>>>>> +    __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
>>>>>> +})
>>>>>
>>>>> While for the latter you merely move the bogus double-leading-
>>>>> underscore macro local variable (which on this occasion I'd
>>>>> like to ask anyway to be changed), you actively introduce a
>>>>> new similar name space violation in the domain_pirq_to_emuirq().
>>>>
>>>> AFAIK, there is nothing in the coding style forbidding your "bogus"
>>>> naming. So I just followed the rest of the code.
>>>
>>> Our coding style document is not to re-iterate C standard rules,
>>> I think, and hence yes, you won't find anything to this effect
>>> there.
>>
>> The fact such code has been added in Xen in the past clearly shows that
>> the coding style is not sufficient to back your point here.
>>
>> So rather than complaining that I don't follow an unwritten rule, you
>> could have suggested it. This would have came accross as less rude.
> 
> If anything I said came across as rude, I'd like to apologize.
> As an explanation (not an excuse), please be aware that I've
> had to request changes to comply to name space rules far too
> often that I would recall towards whom I did send these, or
> that I would assume any of the regular contributors could in
> fact never have noticed this so far.

The double underscore is much lower in my "care" list over 
clean/readable code and consistency. I am sure I have added some on Arm 
in the past few months.

> 
> I do insist on my point though that we, earning our money with
> programming, and hence probably calling ourselves "professional
> programmers", should know and honor basic principles of the
> standards of languages we're using in our day to day work. The
> fact that code violating this had been added to Xen in the past
> does not make this any better; the excuse there may well be
> that it started out as a research project, where such
> considerations may not have mattered all this much. (FAOD I
> explicitly said "basic principles" - I don't expect everyone to
> know every corner case.)
Everyone has different view of what means "professional programmers" and 
what is "basic principles". I could say mine, but I don't think this is 
a really useful discussion to have.

> Do you want me to submit a patch adding something like "It
> probably goes without saying that the underlying language
> standards or specifications are to be honored", perhaps close
> to the top of ./CODING_STYLE?

"standards" and "specifications" are really vague, so this would at 
least need to be expanded with details on standard/specifications we follow.

However, despite what you suggest above, I don't expect everyone 
(including myself) to know those documents by heart. So unless you know 
exactly what you are looking for, this is not going to help much except 
now you can say "You didn't follow the standards".

What we need here is not more specifications to read, but tools that 
help the reviewers/submitters to check coding style (or at least the 
bits we really care).

Cheers,

-- 
Julien Grall

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