Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci
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,
Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci
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
Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci
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
Re: [Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci
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
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
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
[Xen-devel] [PATCH 4/4] xen/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci
From: Julien Grall
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
---
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, >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(>arch.hvm.dpci) )
+if ( !pt_pirq_cleanup_check(pirq_dpci(pirq)) )
return;
}
@@ -2029,7 +2044,7 @@ static inline bool is_free_pirq(const
