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

Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

On 07.10.21 10:42, Jan Beulich wrote:

Hi Jan.

On 06.10.2021 13:22, Oleksandr Tyshchenko wrote:
Changes V4 -> V5:
    - update patch subject and description
    - drop Michal's R-b
    - pass gpaddr_bits via createdomain domctl
      (struct xen_arch_domainconfig)
I'm afraid I can't bring this in line with ...

--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -333,6 +333,11 @@ struct xen_arch_domainconfig {
      uint32_t clock_frequency;
+    /*
+     * OUT
+     * Guest physical address space size
+     */
+    uint8_t gpaddr_bits;
... this being an OUT field. Is this really what Andrew had asked for?
I would have expected the entire struct to be IN (and the comment at
the top of the containing struct in public/domctl.h also suggests so,
i.e. your new field renders that comment stale). gic_version being
IN/OUT is already somewhat in conflict ...
I am sorry but I'm totally confused now, we want the Xen to provide gpaddr_bits to the toolstack, but not the other way around. As I understand the main ask was to switch to domctl for which I wanted to get some clarification on how it would look like... Well, this patch switches to use domctl, and I think exactly as it was suggested at [1] in case if a common one is a difficult to achieve. I have to admit, I felt it was indeed difficult to achieve.

I thought that a comment for struct xen_domctl_createdomain in public/domctl.h was rather related to the struct fields described in the public header than to the arch sub-struct internals described elsewhere. But if my assumption is incorrect, then yes the comment got stale and probably not by changes in current patch, but after adding clock_frequency field (OUT). If so we can add a comment on top of arch field clarifying that internal fields *might* be OUT.

One of the problems with
_any_ of the fields being OUT is that then it is unclear how the output
is intended to be propagated to consumers other than the entity
creating the domain.
If I *properly* understood your concern, we could hide that field in struct libxl__domain_build_state and not expose it to struct libxl_domain_build_info. Shall I? [1] https://lore.kernel.org/xen-devel/093bc1d5-bf6a-da0a-78b5-7a8dd471a063@xxxxxxxxx/


Oleksandr Tyshchenko



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