| Message ID | 1468848357-2331-3-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2016-07-18 13:25+0000, Eric Auger: > Extend kvm_kernel_irq_routing_entry to transport the device id > field, devid. A new flags field makes possible to indicate the > devid is valid. Those additions are used for ARM GICv3 ITS MSI > injection. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Acked-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > v6 -> v7: > - added msi_ prefix to flags and dev_id fields > > v4 -> v5: > - add Christoffer's R-b > > v2 -> v3: > - add flags > > v1 -> v2: > - replace msi_msg field by a struct composed of msi_msg and devid > > RFC -> PATCH: > - reword the commit message after change in first patch (uapi) > --- > include/linux/kvm_host.h | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index c87fe6f..3d2cbb4 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -317,7 +317,11 @@ struct kvm_kernel_irq_routing_entry { > unsigned irqchip; > unsigned pin; > } irqchip; > - struct msi_msg msi; > + struct { > + struct msi_msg msi; > + u32 msi_flags; > + u32 msi_devid; I'd much rather see them as msi.flags and msi.devid. I didn't notice a code that passes struct msi_msg anywhere, so using an ad-hoc struct like irqchip or defining a new one would work fine. Thanks. > + }; > struct kvm_s390_adapter_int adapter; > struct kvm_hv_sint hv_sint; > }; > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/07/16 17:13, Radim Krčmář wrote: > 2016-07-18 13:25+0000, Eric Auger: >> Extend kvm_kernel_irq_routing_entry to transport the device id >> field, devid. A new flags field makes possible to indicate the >> devid is valid. Those additions are used for ARM GICv3 ITS MSI >> injection. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Acked-by: Christoffer Dall <christoffer.dall@linaro.org> >> >> --- >> v6 -> v7: >> - added msi_ prefix to flags and dev_id fields >> >> v4 -> v5: >> - add Christoffer's R-b >> >> v2 -> v3: >> - add flags >> >> v1 -> v2: >> - replace msi_msg field by a struct composed of msi_msg and devid >> >> RFC -> PATCH: >> - reword the commit message after change in first patch (uapi) >> --- >> include/linux/kvm_host.h | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index c87fe6f..3d2cbb4 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -317,7 +317,11 @@ struct kvm_kernel_irq_routing_entry { >> unsigned irqchip; >> unsigned pin; >> } irqchip; >> - struct msi_msg msi; >> + struct { >> + struct msi_msg msi; >> + u32 msi_flags; >> + u32 msi_devid; > > I'd much rather see them as msi.flags and msi.devid. That's not really possible, as msi_msg is the core data structure for MSIs, and nobody really expects this tructure to change. > I didn't notice a code that passes struct msi_msg anywhere, so using an > ad-hoc struct like irqchip or defining a new one would work fine. I guess we could have an arm-specific one: struct arm_msi { struct msi_msg msg; u32 flags; u32 devid; }; and use that. Would that be OK with you? Thanks, M.
2016-07-21 17:54+0100, Marc Zyngier: > On 21/07/16 17:13, Radim Krčmář wrote: >> 2016-07-18 13:25+0000, Eric Auger: >>> Extend kvm_kernel_irq_routing_entry to transport the device id >>> field, devid. A new flags field makes possible to indicate the >>> devid is valid. Those additions are used for ARM GICv3 ITS MSI >>> injection. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Acked-by: Christoffer Dall <christoffer.dall@linaro.org> >>> >>> --- >>> v6 -> v7: >>> - added msi_ prefix to flags and dev_id fields >>> >>> v4 -> v5: >>> - add Christoffer's R-b >>> >>> v2 -> v3: >>> - add flags >>> >>> v1 -> v2: >>> - replace msi_msg field by a struct composed of msi_msg and devid >>> >>> RFC -> PATCH: >>> - reword the commit message after change in first patch (uapi) >>> --- >>> include/linux/kvm_host.h | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> index c87fe6f..3d2cbb4 100644 >>> --- a/include/linux/kvm_host.h >>> +++ b/include/linux/kvm_host.h >>> @@ -317,7 +317,11 @@ struct kvm_kernel_irq_routing_entry { >>> unsigned irqchip; >>> unsigned pin; >>> } irqchip; >>> - struct msi_msg msi; >>> + struct { >>> + struct msi_msg msi; >>> + u32 msi_flags; >>> + u32 msi_devid; >> >> I'd much rather see them as msi.flags and msi.devid. > > That's not really possible, as msi_msg is the core data structure for > MSIs, and nobody really expects this tructure to change. > >> I didn't notice a code that passes struct msi_msg anywhere, so using an >> ad-hoc struct like irqchip or defining a new one would work fine. > > I guess we could have an arm-specific one: > > struct arm_msi { > struct msi_msg msg; > u32 flags; > u32 devid; > }; > > and use that. Would that be OK with you? The feature wants to be arch-neutral, so I would rather ignore struct msi_msg. kvm_kernel_irq_routing_entry practically mirrors routing entries from KVM API and there we have: struct kvm_msi { __u32 address_lo; __u32 address_hi; __u32 data; __u32 flags; __u32 devid; __u8 pad[12]; }; I think that something like struct { u32 address_lo; u32 address_hi; u32 data; u32 flags; u32 devid; } msi; would get us consistency, minimal changes to existing code, no namespace hierarchy, and no "." vs "_" mistakes at a good price of some code duplication and concept separation. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/07/16 18:22, Radim Krčmář wrote: > 2016-07-21 17:54+0100, Marc Zyngier: >> On 21/07/16 17:13, Radim Krčmář wrote: >>> 2016-07-18 13:25+0000, Eric Auger: >>>> Extend kvm_kernel_irq_routing_entry to transport the device id >>>> field, devid. A new flags field makes possible to indicate the >>>> devid is valid. Those additions are used for ARM GICv3 ITS MSI >>>> injection. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> Acked-by: Christoffer Dall <christoffer.dall@linaro.org> >>>> >>>> --- >>>> v6 -> v7: >>>> - added msi_ prefix to flags and dev_id fields >>>> >>>> v4 -> v5: >>>> - add Christoffer's R-b >>>> >>>> v2 -> v3: >>>> - add flags >>>> >>>> v1 -> v2: >>>> - replace msi_msg field by a struct composed of msi_msg and devid >>>> >>>> RFC -> PATCH: >>>> - reword the commit message after change in first patch (uapi) >>>> --- >>>> include/linux/kvm_host.h | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>>> index c87fe6f..3d2cbb4 100644 >>>> --- a/include/linux/kvm_host.h >>>> +++ b/include/linux/kvm_host.h >>>> @@ -317,7 +317,11 @@ struct kvm_kernel_irq_routing_entry { >>>> unsigned irqchip; >>>> unsigned pin; >>>> } irqchip; >>>> - struct msi_msg msi; >>>> + struct { >>>> + struct msi_msg msi; >>>> + u32 msi_flags; >>>> + u32 msi_devid; >>> >>> I'd much rather see them as msi.flags and msi.devid. >> >> That's not really possible, as msi_msg is the core data structure for >> MSIs, and nobody really expects this tructure to change. >> >>> I didn't notice a code that passes struct msi_msg anywhere, so using an >>> ad-hoc struct like irqchip or defining a new one would work fine. >> >> I guess we could have an arm-specific one: >> >> struct arm_msi { >> struct msi_msg msg; >> u32 flags; >> u32 devid; >> }; >> >> and use that. Would that be OK with you? > > The feature wants to be arch-neutral, so I would rather ignore struct > msi_msg. kvm_kernel_irq_routing_entry practically mirrors routing > entries from KVM API and there we have: > > struct kvm_msi { > __u32 address_lo; > __u32 address_hi; > __u32 data; > __u32 flags; > __u32 devid; > __u8 pad[12]; > }; > > I think that something like > > struct { > u32 address_lo; > u32 address_hi; > u32 data; > u32 flags; > u32 devid; > } msi; > > would get us consistency, minimal changes to existing code, no namespace > hierarchy, and no "." vs "_" mistakes at a good price of some code > duplication and concept separation. Fair enough. Eric, can you give this a try? Thanks, M.
Hi, On 21/07/2016 19:25, Marc Zyngier wrote: > On 21/07/16 18:22, Radim Krčmář wrote: >> 2016-07-21 17:54+0100, Marc Zyngier: >>> On 21/07/16 17:13, Radim Krčmář wrote: >>>> 2016-07-18 13:25+0000, Eric Auger: >>>>> Extend kvm_kernel_irq_routing_entry to transport the device id >>>>> field, devid. A new flags field makes possible to indicate the >>>>> devid is valid. Those additions are used for ARM GICv3 ITS MSI >>>>> injection. >>>>> >>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>>> Acked-by: Christoffer Dall <christoffer.dall@linaro.org> >>>>> >>>>> --- >>>>> v6 -> v7: >>>>> - added msi_ prefix to flags and dev_id fields >>>>> >>>>> v4 -> v5: >>>>> - add Christoffer's R-b >>>>> >>>>> v2 -> v3: >>>>> - add flags >>>>> >>>>> v1 -> v2: >>>>> - replace msi_msg field by a struct composed of msi_msg and devid >>>>> >>>>> RFC -> PATCH: >>>>> - reword the commit message after change in first patch (uapi) >>>>> --- >>>>> include/linux/kvm_host.h | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>>>> index c87fe6f..3d2cbb4 100644 >>>>> --- a/include/linux/kvm_host.h >>>>> +++ b/include/linux/kvm_host.h >>>>> @@ -317,7 +317,11 @@ struct kvm_kernel_irq_routing_entry { >>>>> unsigned irqchip; >>>>> unsigned pin; >>>>> } irqchip; >>>>> - struct msi_msg msi; >>>>> + struct { >>>>> + struct msi_msg msi; >>>>> + u32 msi_flags; >>>>> + u32 msi_devid; >>>> >>>> I'd much rather see them as msi.flags and msi.devid. >>> >>> That's not really possible, as msi_msg is the core data structure for >>> MSIs, and nobody really expects this tructure to change. >>> >>>> I didn't notice a code that passes struct msi_msg anywhere, so using an >>>> ad-hoc struct like irqchip or defining a new one would work fine. >>> >>> I guess we could have an arm-specific one: >>> >>> struct arm_msi { >>> struct msi_msg msg; >>> u32 flags; >>> u32 devid; >>> }; >>> >>> and use that. Would that be OK with you? >> >> The feature wants to be arch-neutral, so I would rather ignore struct >> msi_msg. kvm_kernel_irq_routing_entry practically mirrors routing >> entries from KVM API and there we have: >> >> struct kvm_msi { >> __u32 address_lo; >> __u32 address_hi; >> __u32 data; >> __u32 flags; >> __u32 devid; >> __u8 pad[12]; >> }; >> >> I think that something like >> >> struct { >> u32 address_lo; >> u32 address_hi; >> u32 data; >> u32 flags; >> u32 devid; >> } msi; >> >> would get us consistency, minimal changes to existing code, no namespace >> hierarchy, and no "." vs "_" mistakes at a good price of some code >> duplication and concept separation. > > Fair enough. Eric, can you give this a try? Yes I will respin quickly replacing the struct msi_msg msi by the struct proposed by Radim. Thanks Eric > > Thanks, > > M. > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index c87fe6f..3d2cbb4 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -317,7 +317,11 @@ struct kvm_kernel_irq_routing_entry { unsigned irqchip; unsigned pin; } irqchip; - struct msi_msg msi; + struct { + struct msi_msg msi; + u32 msi_flags; + u32 msi_devid; + }; struct kvm_s390_adapter_int adapter; struct kvm_hv_sint hv_sint; };