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

[Oleksandr <olekstysh@xxxxxxxxx>]

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/

--
Regards,

Oleksandr Tyshchenko