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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 07.10.21 15:43, Jan Beulich wrote:

Hi Jan.

> On 07.10.2021 14:30, Oleksandr wrote:
> > On 07.10.21 10:42, Jan Beulich wrote:
> > > 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.
> Sadly the mail you reference isn't the one I was referring to. It's not
> even from Andrew. Unfortunately I also can't seem to be able to locate
> this, i.e. I'm now wondering whether this was under a different subject.
> Julien, in any event, confirmed in a recent reply on this thread that
> there was such a mail (otherwise I would have started wondering whether
> the request was made on irc). In any case it is _that_ mail that would
> need going through again.
I think, this is the email [1] you are referring to. The subject was changed
to reflect changes in the particular version. This is the third 
proposition of this patch
(the first two were with arch and common field in sysctl).


> > 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?
> I'm afraid I'm lost: I didn't talk about the tool stack at all. While
> "consumer" generally means the tool stack, the remark was of more
> abstract nature.
>
> Jan
>
> > [1]
> > https://lore.kernel.org/xen-devel/093bc1d5-bf6a-da0a-78b5-7a8dd471a063@xxxxxxxxx/
[1] 
https://lore.kernel.org/xen-devel/6a2a183d-c9d8-df2a-41aa-b25283fab197@xxxxxxxxx/

--
Regards,

Oleksandr Tyshchenko