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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 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/
> 
>