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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Julien,

On 4/30/2024 5:47 PM, Julien Grall wrote:
> On 30/04/2024 05:00, Henry Wang wrote:
> > Hi Julien,
> Hi Henry,
>
> > 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.
> Thinking a bit more about it, there is another problem with the single 
> hypercall appproach. The MMIOs will be mapped 1:1 to the guest. These 
> region may clash with other part of the layout for domain created by 
> the toolstack
> and dom0less (if the 1:1 option has not been enabled).
>
> I guess for that add, it would be possible to specify the mapping in 
> the Device-Tree. But that would not work for the removal (this may be 
> a different domain).
> On a somewhat similar topic, the number of IRQs supported by the vGIC 
> is fixed at boot. How would that work with this patch?
Seeing your comment here I now realized patch #5 is to address this 
issue. But I think we need to have a complete rework of the original 
patch to make the feature portable. We can continue the discussion in 
patch 5.
> > > 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
> Note that this would attach the devices to dom0 first. Maybe this is 
> why it was decided to merge the two operations? An option would be to 
> allow the devices to be attached to no-one.
> > - Introduce the xl dt-overlay attach <domid> command and respective 
> > domctls to do the device assignment for the overlay to domain.
> We already have domctls to route IRQs and map MMIOs. So do we actually 
> need new domctls?
No I don't think so, like you and Stefano said in the other thread, I 
think I need to split the command to different hypercalls instead of 
only one hypercall and reuse the existing domctl.
Kind regards,
Henry

> Cheers,