Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 06/05/2024 09:32, Henry Wang wrote:
> On 5/1/2024 4:13 AM, Julien Grall wrote:
> > Hi Henry,
> >
> > On 30/04/2024 04:50, Henry Wang wrote:
> > > On 4/25/2024 10:28 PM, Julien Grall wrote:
> > > > > Thanks for your feeedback. After checking the b8577547236f commit 
> > > > > message I think I now understand your point. Do you have any 
> > > > > suggestion about how can I properly add the support to route/remove 
> > > > > the IRQ to running domains? Thanks.
> > > I spent some time going through the GIC/vGIC code and had some 
> > > discussions with Stefano and Stewart during the last couple of days, 
> > > let me see if I can describe the use case properly now to continue 
> > > the discussion:
> > > We have some use cases that requires assigning devices to domains 
> > > after domain boot time. For example, suppose there is an FPGA on the 
> > > board which can simulate a device, and the bitstream for the FPGA is 
> > > provided and programmed after domain boot. So we need a way to assign 
> > > the device to the running domain. This series tries to implement this 
> > > use case by using device tree overlay - users can firstly add the 
> > > overlay to Xen dtb, assign the device in the overlay to a domain by 
> > > the xl command, then apply the overlay to Linux.
> > Thanks for the description! This helps to understand your goal :).
> Thank you very much for spending your time on discussing this and 
> provide these valuable comments!
> > > > I haven't really look at that code in quite a while. I think we need 
> > > > to make sure that the virtual and physical IRQ state matches at the 
> > > > time we do the routing.
> > > > I am undecided on whether we want to simply prevent the action to 
> > > > happen or try to reset the state.
> > > > There is also the question of what to do if the guest is enabling 
> > > > the vIRQ before it is routed.
> > > Sorry for bothering, would you mind elaborating a bit more about the 
> > > two cases that you mentioned above? Commit b8577547236f ("xen/arm: 
> > > Restrict when a physical IRQ can be routed/removed from/to a domain") 
> > > only said there will be undesirable effects, so I am not sure if I 
> > > understand the concerns raised above and the consequences of these 
> > > two use cases.
> > I will try to explain them below after I answer the rest.
> >
> > > I am probably wrong, I think when we add the overlay, we are probably 
> > > fine as the interrupt is not being used before. 
> > What if the DT overlay is unloaded and then reloaded? Wouldn't the 
> > same interrupt be re-used? As a more generic case, this could also be 
> > a new bitstream for the FPGA.
> > But even if the interrupt is brand new every time for the DT overlay, 
> > you are effectively relaxing the check for every user (such as 
> > XEN_DOMCTL_bind_pt_irq). So the interrupt re-use case needs to be 
> > taken into account.
> I agree. I think IIUC, with your explanation here and below, could we 
> simplify the problem to how to properly handle the removal of the IRQ 
> from a running guest, if we always properly remove and clean up the 
> information when remove the IRQ from the guest? In this way, the IRQ can 
> always be viewed as a brand new one when we add it back.
If we can make sure the virtual IRQ and physical IRQ is cleaned then yes.

> Then the only 
> corner case that we need to take care of would be...
Can you clarify whether you say the "only corner case" because you 
looked at the code? Or is it just because I mentioned only one?
> > > Also since we only load the device driver after the IRQ is routed to 
> > > the guest, 
> > This is what a well-behave guest will do. However, we need to think 
> > what will happen if a guest misbehaves. I am not concerned about a 
> > guest only impacting itself, I am more concerned about the case where 
> > the rest of the system is impacted.
> > > I am not sure the guest can enable the vIRQ before it is routed.
> > Xen allows the guest to enable a vIRQ even if there is no pIRQ 
> > assigned. Thanksfully, it looks like the vgic_connect_hw_irq(), in 
> > both the current and new vGIC, will return an error if we are trying 
> > to route a pIRQ to an already enabled vIRQ.
> > But we need to investigate all the possible scenarios to make sure 
> > that any inconsistencies between the physical state and virtual state 
> > (including the LRs) will not result to bigger problem.
> > The one that comes to my mind is: The physical interrupt is 
> > de-assigned from the guest before it was EOIed. In this case, the 
> > interrupt will still be in the LR with the HW bit set. This would 
> > allow the guest to EOI the interrupt even if it is routed to someone 
> > else. It is unclear what would be the impact on the other guest.
> ...same as this case, i.e.
> test_bit(_IRQ_INPROGRESS, &desc->status) || !test_bit(_IRQ_DISABLED, 
> &desc->status)) when we try to remove the IRQ from a running domain.
We already call ->shutdown() which will disable the IRQ. So don't we 
only need to take care of _IRQ_INPROGRESS?
[...]

> we have 3 possible states which can be read from LR for this case : 
> active, pending, pending and active.
> - I don't think we can do anything about the active state, so we should 
> return -EBUSY and reject the whole operation of removing the IRQ from 
> running guest, and user can always retry this operation.
This would mean a malicious/buggy guest would be able to prevent a 
device to be de-assigned. This is not a good idea in particular when the 
domain is dying.
That said, I think you can handle this case. The LR has a bit to 
indicate whether the pIRQ needs to be EOIed. You can clear it and this 
would prevent the guest to touch the pIRQ. There might be other clean-up 
to do in the vGIC datastructure.
Anyway, we don't have to handle removing an active IRQ when the domain 
is still running (although we do when the domain is destroying). But I 
think this would need to be solved before the feature is (security) 
supported.
> - For the pending (and active) case,
Shouldn't the pending and active case handled the same way as the active 
case?
> can we clear the LR and point the 
> LR for the pending_irq to invalid?
LRs can be cleared. You will need to find which vCPU was used for the 
injection and then pause it so the LR can be safely updated.
There will also be some private state to clear. I don't know how easy it 
will be. However, we decided to not do anything for ICPENDR (which 
requires a similar behavior) as this was complex (?) to do with the 
existing vGIC.
I vaguely remember we had some discussions on the ML. I didn't look for 
them though.
Anyway, same as above, this could possibly handled later on. But this 
would probably need to be solved before the feature is (security supported).
Cheers,

--
Julien Grall