Comments

>>> Bhupinder Thakur <bhupinder.thakur@linaro.org> 08/07/17 10:55 AM >>>>@@ -1148,6 +1149,24 @@ struct xen_domctl_psr_cat_op {>uint32_t target; /* IN */>uint64_t data; /* IN/OUT */>};>+>+struct xen_domctl_vuart_op {>+#define XEN_DOMCTL_VUART_OP_INIT 0>+ uint32_t cmd; /* XEN_DOMCTL_VUART_OP_* */>+ domid_t console_domid; /* IN */>+#define XEN_DOMCTL_VUART_TYPE_VPL011 0>+ uint32_t type; /* IN - type of vuart.>+ * Currently only vpl011 supported.>+ */>+ xen_pfn_t gfn; /* IN - guest gfn to be used as a>+ * ring buffer.>+ */>+ evtchn_port_t evtchn; /* OUT - remote port of the event>+ * channel used for sending>+ * ring buffer events.>+ */>+};
Please try to limit the amount of padding needed and make all remaining
padding fields explicit. That should then also make obvious that using
xen_pfn_t in domctl.h is not a good idea (the existing uses are bogus too
afaict - in the getpageframeinfo3 case the handler deals with the differing
width between 32- and 64-bit callers, and the cacheflush one is fine right
now only because it's ARM-only).
Jan

Hi Jan,
On 7 August 2017 at 14:44, Jan Beulich <jbeulich@suse.com> wrote:
>>>> Bhupinder Thakur <bhupinder.thakur@linaro.org> 08/07/17 10:55 AM >>>>>@@ -1148,6 +1149,24 @@ struct xen_domctl_psr_cat_op {>>uint32_t target; /* IN */>>uint64_t data; /* IN/OUT */>>};>>+>>+struct xen_domctl_vuart_op {>>+#define XEN_DOMCTL_VUART_OP_INIT 0>>+ uint32_t cmd; /* XEN_DOMCTL_VUART_OP_* */>>+ domid_t console_domid; /* IN */>>+#define XEN_DOMCTL_VUART_TYPE_VPL011 0>>+ uint32_t type; /* IN - type of vuart.>>+ * Currently only vpl011 supported.>>+ */>>+ xen_pfn_t gfn; /* IN - guest gfn to be used as a>>+ * ring buffer.>>+ */>>+ evtchn_port_t evtchn; /* OUT - remote port of the event>>+ * channel used for sending>>+ * ring buffer events.>>+ */>>+};>> Please try to limit the amount of padding needed and make all remaining> padding fields explicit. That should then also make obvious that using> xen_pfn_t in domctl.h is not a good idea (the existing uses are bogus too> afaict - in the getpageframeinfo3 case the handler deals with the differing> width between 32- and 64-bit callers, and the cacheflush one is fine right> now only because it's ARM-only).
Please correct me if I understood your comment wrongly.
I believe you are referring to re-ordering the fields in the above
structure so that the structure size is minimised.
Should I use type as unsigned long instead of xen_pfn_t in this structure?
Regards,
Bhupinder

>>> On 21.08.17 at 12:28, <bhupinder.thakur@linaro.org> wrote:> Hi Jan,> > On 7 August 2017 at 14:44, Jan Beulich <jbeulich@suse.com> wrote:>>>>> Bhupinder Thakur <bhupinder.thakur@linaro.org> 08/07/17 10:55 AM >>>>>>@@ -1148,6 +1149,24 @@ struct xen_domctl_psr_cat_op {>>>uint32_t target; /* IN */>>>uint64_t data; /* IN/OUT */>>>};>>>+>>>+struct xen_domctl_vuart_op {>>>+#define XEN_DOMCTL_VUART_OP_INIT 0>>>+ uint32_t cmd; /* XEN_DOMCTL_VUART_OP_* */>>>+ domid_t console_domid; /* IN */>>>+#define XEN_DOMCTL_VUART_TYPE_VPL011 0>>>+ uint32_t type; /* IN - type of vuart.>>>+ * Currently only vpl011 supported.>>>+ */>>>+ xen_pfn_t gfn; /* IN - guest gfn to be used as a>>>+ * ring buffer.>>>+ */>>>+ evtchn_port_t evtchn; /* OUT - remote port of the event>>>+ * channel used for sending>>>+ * ring buffer events.>>>+ */>>>+};>>>> Please try to limit the amount of padding needed and make all remaining>> padding fields explicit. That should then also make obvious that using>> xen_pfn_t in domctl.h is not a good idea (the existing uses are bogus too>> afaict - in the getpageframeinfo3 case the handler deals with the differing>> width between 32- and 64-bit callers, and the cacheflush one is fine right>> now only because it's ARM-only).> > Please correct me if I understood your comment wrongly.> > I believe you are referring to re-ordering the fields in the above> structure so that the structure size is minimised.
Yes.
> Should I use type as unsigned long instead of xen_pfn_t in this structure?
No, uint64_aligned_t is what you want.
Jan