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

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

On Mon, Dec 04, 2023 at 06:57:03AM +0000, Chen, Jiqian wrote:
> Hi Daniel P. Smith,
> 
> On 2023/11/30 22:52, Roger Pau Monné wrote:
> > 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.
> 
> Is it like below that I need to add for XSM?
> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index e6ffa948f7..7a289ba5d8 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -91,6 +91,7 @@ static const struct xsm_ops __initconst_cf_clobber 
> dummy_ops = {
>      .resource_plug_pci             = xsm_resource_plug_pci,
>      .resource_unplug_pci           = xsm_resource_unplug_pci,
>      .resource_setup_pci            = xsm_resource_setup_pci,
> +    .resource_config_pci            = xsm_resource_config_pci,

Now that I look at it, using the existing xsm_resource_setup_pci might
be enough, no need to introduce a xsm_resource_config_pci.

Thanks, Roger.