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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 08.10.21 11:13, Jan Beulich wrote:

Hi Jan

> On 07.10.2021 22:23, Stefano Stabellini wrote:
> > On Thu, 7 Oct 2021, Jan Beulich wrote:
> > > On 07.10.2021 15:12, Oleksandr wrote:
> > > > 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.
> > > Well, that's still a mail you sent, not Andrew's. And while I have yours
> > > in my mailbox, I don't have Andrew's for whatever reason.
> > >
> > > Nevertheless there's enough context to be halfway certain that this
> > > wasn't meant as an extension to the create domctl, but rather a separate
> > > new one (merely replacing what you had originally as a sysctl to become
> > > per-domain, to allow returning varying [between domains] values down the
> > > road). I continue to think that if such a field was added to "create",
> > > it would be an input (only).
> > During the Xen Community Call on Tuesday, we briefly spoke about this.
> > Andrew confirmed that what he meant with his original email is to use a
> > domctl. We didn't discuss which domctl specifically.
> >
> > This patch now follows the same pattern of clock_frequency and
> > gic_version (see xen/include/public/arch-arm.h:struct xen_arch_domainconfig).
> > Note that gic_version is an IN/OUT parameter, showing that if in the
> > future we want the ability to set gpaddr_bits (in addition to get
> > gpaddr_bits), it will be possible.
> Well, as said before - I'm not convinced gic_version being IN/OUT is
> appropriate. At the very least a 2nd way to merely retrieve the value
> would seem to be necessary, so that it's not only the party creating
> the guest which would be able to know.
>
> Since here's we're solely after retrieving the value, I don't see
> the point in altering "create". As you say, domctl can be changed,
> and hence at the point this needs to become an input to "create", it
> could easily be added there.
>
> Jan
Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be 
reused to retrieve gpaddr_bits? I don't see why not at the moment, but 
maybe there are some implications/concerns which are invisible to me.
I see that arch_get_domain_info() is present, so the field will be 
common, and each arch will write a value it considers
appropriate. This could be a good compromise to not add an extra domctl 
and to not alter domain_create.

--
Regards,

Oleksandr Tyshchenko