Re: [RFC XEN PATCH v2 1/3] xen/vpci: Clear all vpci status of device

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 30, 2023 at 07:39:38AM -0500, Daniel P. Smith wrote:
> On 11/30/23 07:25, Daniel P. Smith wrote:
> > On 11/30/23 01:22, Chen, Jiqian wrote:
> > > Hi Roger and Daniel P. Smith,
> > > 
> > > On 2023/11/28 22:08, Roger Pau Monné wrote:
> > > > On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote:
> > > > > When a device has been reset on dom0 side, the vpci on Xen
> > > > > side won't get notification, so the cached state in vpci is
> > > > > all out of date compare with the real device state.
> > > > > To solve that problem, this patch add new hypercall to clear
> > > > > all vpci device state. And when reset device happens on dom0
> > > > > side, dom0 can call this hypercall to notify vpci.
> > > > > 
> > > > > Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> > > > > Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> > > > > ---
> > > > >   xen/arch/x86/hvm/hypercall.c  |  1 +
> > > > >   xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++
> > > > >   xen/drivers/pci/physdev.c     | 14 ++++++++++++++
> > > > >   xen/drivers/vpci/vpci.c       |  9 +++++++++
> > > > >   xen/include/public/physdev.h  |  2 ++
> > > > >   xen/include/xen/pci.h         |  1 +
> > > > >   xen/include/xen/vpci.h        |  6 ++++++
> > > > >   7 files changed, 54 insertions(+)
> > > > > 
> > > > > diff --git a/xen/arch/x86/hvm/hypercall.c
> > > > > b/xen/arch/x86/hvm/hypercall.c
> > > > > index eeb73e1aa5..6ad5b4d5f1 100644
> > > > > --- a/xen/arch/x86/hvm/hypercall.c
> > > > > +++ b/xen/arch/x86/hvm/hypercall.c
> > > > > @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd,
> > > > > XEN_GUEST_HANDLE_PARAM(void) arg)
> > > > >       case PHYSDEVOP_pci_mmcfg_reserved:
> > > > >       case PHYSDEVOP_pci_device_add:
> > > > >       case PHYSDEVOP_pci_device_remove:
> > > > > +    case PHYSDEVOP_pci_device_state_reset:
> > > > >       case PHYSDEVOP_dbgp_op:
> > > > >           if ( !is_hardware_domain(currd) )
> > > > >               return -ENOSYS;
> > > > > diff --git a/xen/drivers/passthrough/pci.c
> > > > > b/xen/drivers/passthrough/pci.c
> > > > > index 04d00c7c37..f871715585 100644
> > > > > --- a/xen/drivers/passthrough/pci.c
> > > > > +++ b/xen/drivers/passthrough/pci.c
> > > > > @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> > > > >       return ret;
> > > > >   }
> > > > > +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn)
> > > > 
> > > > You could use pci_sbdf_t here instead of 3 parameters.
> > > Will change in next version, thank you.
> > > 
> > > > 
> > > > I'm however unsure whether we really need this helper just to fetch
> > > > the pdev and call vpci_reset_device_state(), I think you could place
> > > > this logic directly in pci_physdev_op().  Unless there are plans to
> > > > use such logic outside of pci_physdev_op().
> > > If I place the logic of pci_reset_device_state directly in
> > > pci_physdev_op. I think I need to declare vpci_reset_device_state in
> > > pci.h? If it is suitable, I will change in next version.
> > > 
> > > > 
> > > > > +{
> > > > > +    struct pci_dev *pdev;
> > > > > +    int ret = -ENODEV;
> > > > 
> > > > Some XSM check should be introduced fro this operation, as none of the
> > > > existing ones is suitable.  See xsm_resource_unplug_pci() for example.
> > > > 
> > > > xsm_resource_reset_state_pci() or some such I would assume.
> > > I don't know what I should do in XSM function(assume it is
> > > xsm_resource_reset_state_pci).
> > > Hi Daniel P. Smith, could you please give me some suggestions?
> > > Just to check the XSM_PRIV action?
> > > 
> > 
> > Roger, thank you for seeing this and adding me in!
> > 
> > Jiqian, I just wanted to let you know I have seen your post but I have
> > been a little tied up this week. Just with the cursory look, I think
> > Roger is suggesting a new XSM check/hook is warranted.
> > 
> > If you would like to attempt at writing the dummy policy side, mimic
> > xsm_resource_plug_pci in xen/include/xsm/dummy.h and
> > xen/include/xsm/xsm.h, then I can look at handling the FLASK portion
> > next week and provide it to you for inclusion into the series. If you
> > are not comfortable with it, I can look at the whole thing next week.
> > Just let me know what you would be comfortable with.
> 
> Apologies, thinking about for a moment and was thinking the hook should be
> called xsm_resource_config_pci. I would reset as a config operation and
> there might be new ones in the future. I do not believe there is a need to
> have fine grain access control down to individual config operation, thus
> they could all be captured under this one hook. Roger, what are your
> thoughts about this, in particular how you see vpci evolving?

So the configuration space reset should only be done by the domain
that's also capable of triggering the physical device reset, usually
the hardware domain.  I don't think it's possible ATM to allow a
domain different than the hardware domain to perform a PCI reset, as
doing it requires unmediated access to the PCI config space.

So resetting the vPCI state should either be limited to the hardware
domain, or to a pci reset capability explicitly
(xsm_resource_reset_pci or some such?).  Or maybe
xsm_resource_config_pci if that denotes unmediated access to the PCI
config space.

Thanks, Roger.