Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains

[Julien Grall <julien@xxxxxxx>]

On 29/04/2024 04:36, Henry Wang wrote:
> Hi Jan, Julien, Stefano,
Hi Henry,

> On 4/24/2024 2:05 PM, Jan Beulich wrote:
> > On 24.04.2024 05:34, Henry Wang wrote:
> > > --- a/xen/include/public/sysctl.h
> > > +++ b/xen/include/public/sysctl.h
> > > @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
> > >   #define XEN_SYSCTL_DT_OVERLAY_ADD                   1
> > >   #define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
> > >       uint8_t overlay_op;                     /* IN: Add or remove. */
> > > -    uint8_t pad[3];                         /* IN: Must be zero. */
> > > +    bool domain_mapping;                    /* IN: True of False. */
> > > +    uint8_t pad[2];                         /* IN: Must be zero. */
> > > +    uint32_t domain_id;
> > >   };
> > If you merely re-purposed padding fields, all would be fine without
> > bumping the interface version. Yet you don't, albeit for an unclear
> > reason: Why uint32_t rather than domid_t? And on top of that - why a
> > separate boolean when you could use e.g. DOMID_INVALID to indicate
> > "no domain mapping"?
> I think both of your suggestion make great sense. I will follow the 
> suggestion in v2.
> > That said - anything taking a domain ID is certainly suspicious in a
> > sysctl. Judging from the description you really mean this to be a
> > domctl. Anything else will require extra justification.
> I also think a domctl is better. I had a look at the history of the 
> already merged series, it looks like in the first version of merged part 
> 1 [1], the hypercall was implemented as the domctl in the beginning but 
> later in v2 changed to sysctl. I think this makes sense as the scope of 
> that time is just to make Xen aware of the device tree node via Xen 
> device tree.
> However this is now a problem for the current part where the scope (and 
> the end goal) is extended to assign the added device to Linux Dom0/DomU 
> via device tree overlays. I am not sure which way is better, should we 
> repurposing the sysctl to domctl or maybe add another domctl (I am 
> worrying about the duplication because basically we need the same sysctl 
> functionality but now with a domid in it)? What do you think?
I am not entirely sure this is a good idea to try to add the device in 
Xen and attach it to the guests at the same time. Imagine the following 
situation:
1) Add and attach devices
2) The domain is rebooted
3) Detach and remove devices

After step 2, you technically have a new domain. You could have also a 
case where this is a completely different guest. So the flow would look 
a little bit weird (you create the DT overlay with domain A but remove 
with domain B).
So, at the moment, it feels like the add/attach (resp detech/remove) 
operations should happen separately.
Can you clarify why you want to add devices to Xen and attach to a guest 
within a single hypercall?
Cheers,

--
Julien Grall