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

[Jan Beulich <jbeulich@xxxxxxxx>]

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