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

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 08/05/2024 08:49, Henry Wang wrote:
> On 5/8/2024 5:54 AM, Julien Grall wrote:
> > Hi Henry,
> > > > 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?
> Well, I indeed checked the code and to my best knowledge the corner case 
> that you pointed out would be the only one I can think of.
Ok :). I was just checking you had a look as well.

[...]

> > > 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.
> I probably misunderstood this sentence, do you mean the EOI bit in the 
> pINTID field? I think this bit is only available when the HW bit of LR 
> is 0, but in our case the HW is supposed to be 1 (as indicated as your 
> previous comment). Would you mind clarifying a bit more? Thanks!
You are right, ICH_LR.HW will be 1 for physical IRQ routed to a guest. 
What I was trying to explain is this bit could be cleared (with 
ICH_LR.pINTD adjusted).
Cheers,

--
Julien Grall