[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC XEN PATCH v2 1/3] xen/vpci: Clear all vpci status of device
On 2023/12/4 19:10, Roger Pau Monné wrote: > 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. Ok, thank you. I will add xsm_resource_setup_pci check into PHYSDEVOP_pci_device_state_reset next step. > > Thanks, Roger. -- Best regards, Jiqian Chen.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |