[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.