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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 30 Apr 2024, Henry Wang wrote:
> Hi Julien,
> 
> On 4/30/2024 1:34 AM, Julien Grall wrote:
> > 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?
> 
> Sorry I don't know if there is any specific thoughts on the design of using a
> single hypercall to do both add devices to Xen device tree and assign the
> device to the guest. In fact seeing your above comments, I think separating
> these two functionality to two xl commands using separated hypercalls would
> indeed be a better idea. Thank you for the suggestion!
> 
> To make sure I understand correctly, would you mind confirming if below
> actions for v2 make sense to you? Thanks!
> - Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to add/remove
> overlay to Xen device tree
> - Introduce the xl dt-overlay attach <domid> command and respective domctls to
> do the device assignment for the overlay to domain.

I think two hypercalls is OK. The original idea was to have a single xl
command to do the operation for user convenience (even that is not a
hard requirement) but that can result easily in two hypercalls.